Re: It seems there is a very tight character count limit in .gitconfig
On Wed, Jan 08, 2014 at 02:59:37PM +0800, Li Zhang wrote: > I tried to add url xxx insteadof xxx in .gitconfig. If the length of > url exceed 125, git will not work. > I am using Ubuntu. The default version is 1.7.9.5. Maybe the latest > version solve this already. Yes, this was fixed in 0971e99 (Remove the hard coded length limit on variable names in config files, 2012-09-30). Git v1.8.0.1 and higher contain that commit. -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
It seems there is a very tight character count limit in .gitconfig
I tried to add url xxx insteadof xxx in .gitconfig. If the length of url exceed 125, git will not work. I am using Ubuntu. The default version is 1.7.9.5. Maybe the latest version solve this already. -- 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: Preferred local submodule branches
On Tue, Jan 07, 2014 at 07:47:08PM -0800, W. Trevor King wrote: > #git checkout --recurse-submodules master > ( # 'git checkout --recurse-submodules' doesn't exist yet [2,3]. > # Even with that patch, 'git checkout' won't respect > # submodule..local-branch without further work. > git checkout master && > cd submod && > git checkout master # don't pull in our my-feature work > ) > git submodule update --remote && > git commit -am 'Catch submod up with Subproject v2' && > # update the my-feature branch > git checkout my-feature > ( # 'git checkout' doesn't mess with submodules > cd submod && > git checkout my-feature > ) Oops, the my-feature checkout block should have been: #git checkout --recurse-submodules my-feature ( # 'git checkout --recurse-submodules' doesn't exist yet... git checkout my-feature && cd submod && git checkout my-feature ) mirroring the earlier master checkout block. Sorry for the sloppy editing. Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
[PATCH v3 3/5] refs: teach for_each_ref a flag to avoid recursion
On Tue, Jan 07, 2014 at 06:58:50PM -0500, Jeff King wrote:
> + if (flags & DO_FOR_EACH_NO_RECURSE) {
> + struct ref_dir *subdir = get_ref_dir(entry);
> + sort_ref_dir(subdir);
> + retval = do_for_each_entry_in_dir(subdir, 0,
Obviously this is totally wrong and inverts the point of the flag. And
causes something like half of the test suite to fail.
Michael was nice enough to point it out to me off-list, but well, I have
to face the brown paper bag at some point. :) In my defense, it was a
last minute refactor before going to dinner. That is what I get for
rushing out the series.
Here's a fixed version of patch 3/5.
-- >8 --
Subject: refs: teach for_each_ref a flag to avoid recursion
The normal for_each_ref traversal descends into
subdirectories, returning each ref it finds. However, in
some cases we may want to just iterate over the top-level of
a certain part of the tree.
The introduction of the "flags" option is a little
mysterious. We already have a "flags" option that gets stuck
in a callback struct and ends up interpreted in do_one_ref.
But the traversal itself does not currently have any flags,
and it needs to know about this new flag.
We _could_ introduce this as a completely separate flag
parameter. But instead, we simply put both flag types into a
single namespace, and make it available at both sites. This
is simple, and given that we do not have a proliferation of
flags (we have had exactly one until now), it is probably
sufficient.
Signed-off-by: Jeff King
---
refs.c | 61 ++---
1 file changed, 38 insertions(+), 23 deletions(-)
diff --git a/refs.c b/refs.c
index 3926136..b70b018 100644
--- a/refs.c
+++ b/refs.c
@@ -589,6 +589,8 @@ static void sort_ref_dir(struct ref_dir *dir)
/* Include broken references in a do_for_each_ref*() iteration: */
#define DO_FOR_EACH_INCLUDE_BROKEN 0x01
+/* Do not recurse into subdirs, just iterate at a single level. */
+#define DO_FOR_EACH_NO_RECURSE 0x02
/*
* Return true iff the reference described by entry can be resolved to
@@ -661,7 +663,8 @@ static int do_one_ref(struct ref_entry *entry, void
*cb_data)
* called for all references, including broken ones.
*/
static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset,
- each_ref_entry_fn fn, void *cb_data)
+ each_ref_entry_fn fn, void *cb_data,
+ int flags)
{
int i;
assert(dir->sorted == dir->nr);
@@ -669,9 +672,13 @@ static int do_for_each_entry_in_dir(struct ref_dir *dir,
int offset,
struct ref_entry *entry = dir->entries[i];
int retval;
if (entry->flag & REF_DIR) {
- struct ref_dir *subdir = get_ref_dir(entry);
- sort_ref_dir(subdir);
- retval = do_for_each_entry_in_dir(subdir, 0, fn,
cb_data);
+ if (!(flags & DO_FOR_EACH_NO_RECURSE)) {
+ struct ref_dir *subdir = get_ref_dir(entry);
+ sort_ref_dir(subdir);
+ retval = do_for_each_entry_in_dir(subdir, 0,
+ fn, cb_data,
+ flags);
+ }
} else {
retval = fn(entry, cb_data);
}
@@ -691,7 +698,8 @@ static int do_for_each_entry_in_dir(struct ref_dir *dir,
int offset,
*/
static int do_for_each_entry_in_dirs(struct ref_dir *dir1,
struct ref_dir *dir2,
-each_ref_entry_fn fn, void *cb_data)
+each_ref_entry_fn fn, void *cb_data,
+int flags)
{
int retval;
int i1 = 0, i2 = 0;
@@ -702,10 +710,12 @@ static int do_for_each_entry_in_dirs(struct ref_dir *dir1,
struct ref_entry *e1, *e2;
int cmp;
if (i1 == dir1->nr) {
- return do_for_each_entry_in_dir(dir2, i2, fn, cb_data);
+ return do_for_each_entry_in_dir(dir2, i2, fn, cb_data,
+ flags);
}
if (i2 == dir2->nr) {
- return do_for_each_entry_in_dir(dir1, i1, fn, cb_data);
+ return do_for_each_entry_in_dir(dir1, i1, fn, cb_data,
+ flags);
}
e1 = dir1->entries[i1];
e2 = dir2->entries[i2];
@@ -713,12 +723,15 @@ static int do_for_each_entry_in_dirs(struct ref_dir *dir1,
if (cmp == 0) {
Re: Preferred local submodule branches
On Wed, Jan 08, 2014 at 03:12:44AM +0100, Francesco Pretto wrote:
> 2014/1/8 W. Trevor King :
> > Note that I've moved away from “submodule..branch
> > set means attached” towards “we should set per-superproject-branch
> > submodule..local-branch explicitly” [1].
>
> Honestly, I'm having an hard time to follow this thread.
I tried to refocus things (with a new subject) in this sub-thread.
Hopefully that helps make the discussion more linear ;).
> Also, you didn't update the patch.
I'm waiting [1] to see how the C-level checkout by Jens and Jonathan
progresses [2,3] before writing more code.
> If you were endorsed by someone (Junio, Heiko, ...) for the
> "submodule..local-branch" feature please show me where.
As far as I know, no-one else has endorsed this idea (yet :). Heiko
has expressed concern [4], but not convincingly enough (yet :) to win
me over ;).
> I somehow understand the point of the
> "submodule..local-branch" property, but I can't "see" the the
> workflow. Please, show me some hypothetical scripting example with
> as much complete as possible workflow (creation, developer update,
> mantainers creates feature branch, developer update, developer
> attach to another branch).
I've put this at the bottom of the message to avoid bothering the
tl;dr crowd, although they have probably long since tuned us out ;).
> Also, consider I proposed to support the attached HEAD path to
> reduce complexity and support a simpler use case for git
> submodules. I would be disappointed if the complexity is reduced in
> a way and augmented in another.
Agreed. I think we're all looking for the least-complex solution that
covers all (or most) reasonable workflows.
> > On Wed, Jan 08, 2014 at 01:17:49AM +0100, Francesco Pretto wrote:
> >> # Attach the submodule HEAD to .
> >> # Also set ".git/config" 'submodule..branch' to
> >> $ git submodule head -b --attach
> > [...]
> > I also prefer 'checkout' to 'head', because 'checkout'
> > already exists in non-submodule Git for switching between local
> > branches.
>
> I can agree with similarity to other git commands, but 'checkout'
> does not give me the idea of something that writes to ".git/config"
> or ".gitmodules".
Neither does 'head'. We have precedence in 'git submodule add' for
embracing and extending a core git command with additional .gitmodules
manipulation. I think it's easier to pick up the submodule jargon
when we add submodule-specific side-effects to submodule-specific
commands named after their core analogs than it would be if we pick
unique names for the submodule-specific commands.
> >> # Unset ".git/config" 'submodule..branch'
> >> # Also attach or detach the HEAD according to what is in ".gitmodules":
> >> # with Trevor's patch 'submodule..branch' set means attached,
> >> # unset means detached
> >> $ git submodule head --reset
> >
> > To me this reads “always detach HEAD” (because it unsets
> > submodule..branch, and submodule..branch unset means
> > detached).
>
> I disagree: this would remove only the value in ".git/config". If the
> value is till present in ".gitmodules", as I wrote above, the behavior
> of what is in the index should be respected as for the other
> properties. Also it gives a nice meaning to a switch like --reset :
> return to how it was before.
Ah, that makes more sense. I had confused .git/config with
“.gitmodules and .git/config”.
> >> NOTE: feature branch part!
> >>
> >> # Set ".gitmodules" 'submodule..branch' to
> >> $ git submodule head -b --attach --index
> >>
> >> # Unset ".gitmodules" 'submodule..branch'
> >> $ git submodule head --reset --index
> >> -
> >
> > These are just manipulating .gitmodules. I think we also need
> > per-superproject-branch configs under the superproject's .git/ for
> > developer overrides.
>
> I disagree: in my idea the --index switch is a maintainer only command
> to modify the behavior of the developers and touch only indexed files
> (.gitmodules, or create a new submodule branch). It expressly don't
> touch .git/config.
Something that just touches the config files is syntactic sugar, so I
avoided a more detailed review and moved on to address what I saw as a
more fundamental issue (preferred submodule local branches on a
per-superproject-branch level).
Here's a detailed workflow for the {my-feature, my-feature, master}
example I roughed out before [5].
# create the subproject
mkdir subproject &&
(
cd subproject &&
git init &&
echo 'Hello, world' > README &&
git add README &&
git commit -m 'Subproject v1'
) &&
# create the superproject
mkdir superproject
(
cd superproject &&
git init &&
git submodule add ../subproject submod &&
git config -f .gitmodules submodule.submod.update merge &&
git commit -am 'Superproject v1' &&
( # 'submodule update' doesn't look in .gitmodules (yet [6]) for a
# default update mode. Copy submodule.submod
Re: Re: [RFC v2] submodule: Respect requested branch on all clones
2014/1/8 W. Trevor King : > Note that I've moved away from “submodule..branch > set means attached” towards “we should set per-superproject-branch > submodule..local-branch explicitly” [1]. > Honestly, I'm having an hard time to follow this thread. Also, you didn't update the patch. If you were endorsed by someone (Junio, Heiko, ...) for the "submodule..local-branch" feature please show me where. I somehow understand the point of the "submodule..local-branch" property, but I can't "see" the the workflow. Please, show me some hypothetical scripting example with as much complete as possible workflow (creation, developer update, mantainers creates feature branch, developer update, developer attach to another branch). Also, consider I proposed to support the attached HEAD path to reduce complexity and support a simpler use case for git submodules. I would be disappointed if the complexity is reduced in a way and augmented in another. > On Wed, Jan 08, 2014 at 01:17:49AM +0100, Francesco Pretto wrote: >> # Attach the submodule HEAD to . >> # Also set ".git/config" 'submodule..branch' to >> $ git submodule head -b --attach > [...] > I also prefer 'checkout' to 'head', because 'checkout' > already exists in non-submodule Git for switching between local > branches. > I can agree with similarity to other git commands, but 'checkout' does not give me the idea of something that writes to ".git/config" or ".gitmodules". >> # Attach the submodule HEAD to 'submodule..branch'. >> # If it does not exists defaults to /master >> $ git submodule head --attach > > Defaulting to the configured local branch is fine, but I think it > should default to 'master' if no local branch is configured. This > should not have anything to do with remote-tracking branches (that's > what 'submodule update' already handles). I don't understand why > remote-tracking-branch integration keeps getting mixed up with > local-branch checkout. > Yep, it should default to "master", my fault. >> # Unset ".git/config" 'submodule..branch' >> # Also attach or detach the HEAD according to what is in ".gitmodules": >> # with Trevor's patch 'submodule..branch' set means attached, >> # unset means detached >> $ git submodule head --reset > > To me this reads “always detach HEAD” (because it unsets > submodule..branch, and submodule..branch unset means > detached). I disagree: this would remove only the value in ".git/config". If the value is till present in ".gitmodules", as I wrote above, the behavior of what is in the index should be respected as for the other properties. Also it gives a nice meaning to a switch like --reset : return to how it was before. >> NOTE: feature branch part! >> >> # Set ".gitmodules" 'submodule..branch' to >> $ git submodule head -b --attach --index >> >> # Unset ".gitmodules" 'submodule..branch' >> $ git submodule head --reset --index >> - > > These are just manipulating .gitmodules. I think we also need > per-superproject-branch configs under the superproject's .git/ for > developer overrides. > I disagree: in my idea the --index switch is a maintainer only command to modify the behavior of the developers and touch only indexed files (.gitmodules, or create a new submodule branch). It expressly don't touch .git/config. -- 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: Re: [RFC v2] submodule: Respect requested branch on all clones
On Wed, Jan 08, 2014 at 01:17:49AM +0100, Francesco Pretto wrote:
> # Attach the submodule HEAD to .
> # Also set ".git/config" 'submodule..branch' to
> $ git submodule head -b --attach
I prefer submodule..local-branch for the submodule's local
branch name. I also prefer 'checkout' to 'head', because 'checkout'
already exists in non-submodule Git for switching between local
branches.
> # Attach the submodule HEAD to 'submodule..branch'.
> # If it does not exists defaults to /master
> $ git submodule head --attach
Defaulting to the configured local branch is fine, but I think it
should default to 'master' if no local branch is configured. This
should not have anything to do with remote-tracking branches (that's
what 'submodule update' already handles). I don't understand why
remote-tracking-branch integration keeps getting mixed up with
local-branch checkout.
> # Unset ".git/config" 'submodule..branch'
> # Also attach or detach the HEAD according to what is in ".gitmodules":
> # with Trevor's patch 'submodule..branch' set means attached,
> # unset means detached
> $ git submodule head --reset
To me this reads “always detach HEAD” (because it unsets
submodule..branch, and submodule..branch unset means
detached). Note that I've moved away from “submodule..branch
set means attached” towards “we should set per-superproject-branch
submodule..local-branch explicitly” [1].
> NOTE: feature branch part!
>
> # Set ".gitmodules" 'submodule..branch' to
> $ git submodule head -b --attach --index
>
> # Unset ".gitmodules" 'submodule..branch'
> $ git submodule head --reset --index
> -
These are just manipulating .gitmodules. I think we also need
per-superproject-branch configs under the superproject's .git/ for
developer overrides.
> What do you think? Better?
I don't think so. To elaborate the idea I sketched out here [2], say
you want:
Superproject branch Submodule branch Upstream branch
=== ===
master mastermaster
super-featuremastermaster
my-feature my-featuremaster
other-featureother-feature other-feature
That's only going to work with per-superproject-branch configs for
both the local and remote branches. Using the same name for both
local and remote branches does not work.
Let me motivate each of the combinations in the above table:
* master, master, master: The stable trunk.
* super-feature, master, master: A superproject feature that works
with the stock submodule.
* my-feature, my-feature, master: A superproject feature that needs an
improved submodule, but wants to integrate upstream master changes
during development.
* other-feature, other-feature, other-feature: A superproject feature
that needs an improved submodule, and wants to integrate
other-feature changes that are also being developed upstream.
The deal-breaker for recycling submodule..branch to also mean
the local branch name is the {my-feature, my-feature, master} case.
Do we want to force submodule developers to always match the upstream
name of the feature branch they'd like to integrate with? What if
there is no upstream my-feature branch (and the superproject developer
pushes submodule branches upstream via email)? Making the local
branch name independently configurable avoids these issues with a
minimal increase in complexity.
Cheers,
Trevor
[1]: http://article.gmane.org/gmane.comp.version-control.git/240177
[2]: http://article.gmane.org/gmane.comp.version-control.git/240180
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy
signature.asc
Description: OpenPGP digital signature
Re: Re: [RFC v2] submodule: Respect requested branch on all clones
2014/1/7 Heiko Voigt : > One thing is missing though (and I think thats where Francesco came > from): What if the developer already has a detached HEAD in the > submodule? > > How does he attach to a branch? For this we need something similar to > Francescos attach/detach or Trevors submodule checkout with Junio's checkout > HEAD~0 from here[1]. > > I am still undecided how we should call it. Because of my > > Idea for feature branch support > - --- > > For the branch attaching feature I would also like something that can actually > modify .git/config and for me more importantly .gitmodules. > > So e.g. if I want to work on a longer lived feature branch in a submodule > which > I need in a feature branch in the superproject I would do something like this: > > $ git submodule checkout --gitmodules --merge -b hv/my-cool-feature > I said in another thread I said to Junio am not pursuing --attach|--detach anymore, but seeing that now everybody seem to be excited about attached HEAD here we are... Heiko, it's all day I think this syntax: it supports your above "git submodule checkout" and more. Take attention at the feature branch part! NOTE: the following seems to me compatible with Trevor's "submodule..branch means attached" patch. git submodule head The full syntax is the sum of the following ones: git submodule head [-b ] [--attach] [--] [...] git submodule head [-b ] [--attach] --index [--] [...] git submodule head --reset [--] [...] git submodule head --reset --index [--] [...] (NOTE: --index should be the same as Heiko's above --gitmodules, it means -> touch .gitmodules) All the switches combinations follow, explained: # Attach the submodule HEAD to . # Also set ".git/config" 'submodule..branch' to $ git submodule head -b --attach # Attach the submodule HEAD to 'submodule..branch'. # If it does not exists defaults to /master $ git submodule head --attach # Unset ".git/config" 'submodule..branch' # Also attach or detach the HEAD according to what is in ".gitmodules": # with Trevor's patch 'submodule..branch' set means attached, # unset means detached $ git submodule head --reset NOTE: feature branch part! # Set ".gitmodules" 'submodule..branch' to $ git submodule head -b --attach --index # Unset ".gitmodules" 'submodule..branch' $ git submodule head --reset --index - Also note that a --detach switch is not needed with Trevor's patch. To resync to a dettached HEAD workflow, when 'submodule..branch' is unset in ".gitmodule", --reset (without --index) should be enough. What do you think? Better? Thank you, Francesco -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] stash: handle specifying stashes with spaces
Junio C Hamano pobox.com> writes:
>
> Thomas Rast thomasrast.ch> writes:
>
> > Junio C Hamano pobox.com> writes:
> >
> >>
> >> This is brittle. If new tests are added before this, the test_tick
> >> will give you different timestamp and this test will start failing.
> >>
> >> Perhaps grab the timestamp out of the stash that was created [...]
> >
> > Hmm, now that I stare at this, why not simply use something like
> >
> > git stash apply "stash { 0 }"
> >
> > It seems to refer to the same as stash {0} as one would expect, while
> > still triggering the bug with unpatched git-stash.
>
> Yeah, that is fine as well. For the record, here is what I
> tentatively queued.
>
I completely agree that it's brittle. I mentioned it when I submitted v1
but at the time it didn't occur to me that new tests might be added
before it. And of course I agree with your proposed changes to the test.
I must say I like Thomas' solution because of its simplicity and the
whole test could be made much shorter: just create stash and try to pop
it.
But it's seems the spaces trigger some other way of interpreting the
selector. In my git.git, git rev-parse HEAD{0} gives me the same result
as HEAD@{ 0 } but HEAD@{1} and HEAD@{ 1 } are different. Is this
intentional? If not, can this impact the reliability of the test if we
use HEAD@{ 0 } ?
Thanks for queuing!
Regards,
Øsse
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/5] get_sha1: speed up ambiguous 40-hex test
Since 798c35f (get_sha1: warn about full or short object
names that look like refs, 2013-05-29), a 40-hex sha1 causes
us to call dwim_ref on the result, on the off chance that we
have a matching ref. This can cause a noticeable slow-down
when there are a large number of objects. E.g., on
linux.git:
[baseline timing]
$ best-of-five git rev-list --all --pretty=raw
real0m3.996s
user0m3.900s
sys 0m0.100s
[same thing, but calling get_sha1 on each commit from stdin]
$ git rev-list --all >commits
$ best-of-five -i commits git rev-list --stdin --pretty=raw
real0m7.862s
user0m6.108s
sys 0m1.760s
The problem is that each call to dwim_ref individually stats
the possible refs in refs/heads, refs/tags, etc. In the
common case, there are no refs that look like sha1s at all.
We can therefore do the same check much faster by loading
all ambiguous-looking candidates once, and then checking our
index for each object.
This is technically more racy (somebody might create such a
ref after we build our index), but that's OK, as it's just a
warning (and we provide no guarantees about whether a
simultaneous process ran before or after the ref was created
anyway).
Here is the time after this patch, which implements the
strategy described above:
$ best-of-five -i commits git rev-list --stdin --pretty=raw
real0m4.966s
user0m4.776s
sys 0m0.192s
We still pay some price to read the commits from stdin, but
notice the system time is much lower, as we are avoiding
hundreds of thousands of stat() calls.
Signed-off-by: Jeff King
---
I wanted to make the ref traversal as cheap as possible, hence the
NO_RECURSE flag I added. I thought INCLUDE_BROKEN used to not open up
the refs at all, but it looks like it does these days. I wonder if that
is worth changing or not.
refs.c | 47 +++
refs.h | 2 ++
sha1_name.c | 4 +---
3 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index ca854d6..cddd871 100644
--- a/refs.c
+++ b/refs.c
@@ -4,6 +4,7 @@
#include "tag.h"
#include "dir.h"
#include "string-list.h"
+#include "sha1-array.h"
/*
* Make sure "ref" is something reasonable to have under ".git/refs/";
@@ -2042,6 +2043,52 @@ int dwim_log(const char *str, int len, unsigned char
*sha1, char **log)
return logs_found;
}
+static int check_ambiguous_sha1_ref(const char *refname,
+ const unsigned char *sha1,
+ int flags,
+ void *data)
+{
+ unsigned char tmp_sha1[20];
+ if (strlen(refname) == 40 && !get_sha1_hex(refname, tmp_sha1))
+ sha1_array_append(data, tmp_sha1);
+ return 0;
+}
+
+static void build_ambiguous_sha1_ref_index(struct sha1_array *idx)
+{
+ const char **rule;
+
+ for (rule = ref_rev_parse_rules; *rule; rule++) {
+ const char *prefix = *rule;
+ const char *end = strstr(prefix, "%.*s");
+ char *buf;
+
+ if (!end)
+ continue;
+
+ buf = xmemdupz(prefix, end - prefix);
+ do_for_each_ref(&ref_cache, buf, check_ambiguous_sha1_ref,
+ end - prefix,
+ DO_FOR_EACH_INCLUDE_BROKEN |
+ DO_FOR_EACH_NO_RECURSE,
+ idx);
+ free(buf);
+ }
+}
+
+int sha1_is_ambiguous_with_ref(const unsigned char *sha1)
+{
+ struct sha1_array idx = SHA1_ARRAY_INIT;
+ static int loaded;
+
+ if (!loaded) {
+ build_ambiguous_sha1_ref_index(&idx);
+ loaded = 1;
+ }
+
+ return sha1_array_lookup(&idx, sha1) >= 0;
+}
+
static struct ref_lock *lock_ref_sha1_basic(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p)
diff --git a/refs.h b/refs.h
index 87a1a79..c7d5f89 100644
--- a/refs.h
+++ b/refs.h
@@ -229,4 +229,6 @@ int update_refs(const char *action, const struct ref_update
**updates,
extern int parse_hide_refs_config(const char *var, const char *value, const
char *);
extern int ref_is_hidden(const char *);
+int sha1_is_ambiguous_with_ref(const unsigned char *sha1);
+
#endif /* REFS_H */
diff --git a/sha1_name.c b/sha1_name.c
index a5578f7..f83ecb7 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -452,13 +452,11 @@ static int get_sha1_basic(const char *str, int len,
unsigned char *sha1)
if (len == 40 && !get_sha1_hex(str, sha1)) {
if (warn_ambiguous_refs && warn_on_object_refname_ambiguity) {
- refs_found = dwim_ref(str, len, tmp_sha1, &real_ref);
- if (refs_found > 0) {
+ if (sha1_is_ambiguous_with_ref(sha1)) {
warning(warn_msg,
[PATCH v2 5/5] get_sha1: drop object/refname ambiguity flag
Now that our object/refname ambiguity test is much faster
(thanks to the previous commit), there is no reason for code
like "cat-file --batch-check" to turn it off. Here are
before and after timings with this patch (on git.git):
$ git rev-list --objects --all | cut -d' ' -f1 >objects
[with flag]
$ best-of-five -i objects ./git cat-file --batch-check
real0m0.392s
user0m0.368s
sys 0m0.024s
[without flag, without speedup; i.e., pre-25fba78]
$ best-of-five -i objects ./git cat-file --batch-check
real0m1.652s
user0m0.904s
sys 0m0.748s
[without flag, with speedup]
$ best-of-five -i objects ./git cat-file --batch-check
real0m0.388s
user0m0.356s
sys 0m0.028s
So the new implementation does just as well as we did with
the flag turning the whole thing off (better actually, but
that is within the noise).
Signed-off-by: Jeff King
---
builtin/cat-file.c | 9 -
cache.h| 1 -
environment.c | 1 -
sha1_name.c| 2 +-
4 files changed, 1 insertion(+), 12 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index ce79103..afba21f 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -285,15 +285,6 @@ static int batch_objects(struct batch_options *opt)
if (opt->print_contents)
data.info.typep = &data.type;
- /*
-* We are going to call get_sha1 on a potentially very large number of
-* objects. In most large cases, these will be actual object sha1s. The
-* cost to double-check that each one is not also a ref (just so we can
-* warn) ends up dwarfing the actual cost of the object lookups
-* themselves. We can work around it by just turning off the warning.
-*/
- warn_on_object_refname_ambiguity = 0;
-
while (strbuf_getline(&buf, stdin, '\n') != EOF) {
if (data.split_on_whitespace) {
/*
diff --git a/cache.h b/cache.h
index ce377e1..73afc38 100644
--- a/cache.h
+++ b/cache.h
@@ -566,7 +566,6 @@ extern int assume_unchanged;
extern int prefer_symlink_refs;
extern int log_all_ref_updates;
extern int warn_ambiguous_refs;
-extern int warn_on_object_refname_ambiguity;
extern int shared_repository;
extern const char *apply_default_whitespace;
extern const char *apply_default_ignorewhitespace;
diff --git a/environment.c b/environment.c
index 3c76905..c59f6d4 100644
--- a/environment.c
+++ b/environment.c
@@ -22,7 +22,6 @@ int prefer_symlink_refs;
int is_bare_repository_cfg = -1; /* unspecified */
int log_all_ref_updates = -1; /* unspecified */
int warn_ambiguous_refs = 1;
-int warn_on_object_refname_ambiguity = 1;
int repository_format_version;
const char *git_commit_encoding;
const char *git_log_output_encoding;
diff --git a/sha1_name.c b/sha1_name.c
index f83ecb7..b9aaf74 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -451,7 +451,7 @@ static int get_sha1_basic(const char *str, int len,
unsigned char *sha1)
int at, reflog_len, nth_prior = 0;
if (len == 40 && !get_sha1_hex(str, sha1)) {
- if (warn_ambiguous_refs && warn_on_object_refname_ambiguity) {
+ if (warn_ambiguous_refs) {
if (sha1_is_ambiguous_with_ref(sha1)) {
warning(warn_msg, len, str);
if (advice_object_name_warning)
--
1.8.5.2.500.g8060133
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/5] refs: teach for_each_ref a flag to avoid recursion
The normal for_each_ref traversal descends into
subdirectories, returning each ref it finds. However, in
some cases we may want to just iterate over the top-level of
a certain part of the tree.
The introduction of the "flags" option is a little
mysterious. We already have a "flags" option that gets stuck
in a callback struct and ends up interpreted in do_one_ref.
But the traversal itself does not currently have any flags,
and it needs to know about this new flag.
We _could_ introduce this as a completely separate flag
parameter. But instead, we simply put both flag types into a
single namespace, and make it available at both sites. This
is simple, and given that we do not have a proliferation of
flags (we have had exactly one until now), it is probably
sufficient.
Signed-off-by: Jeff King
---
I think the flags thing is OK as explained above, but Michael may have a
different suggestion for refactoring.
refs.c | 61 ++---
1 file changed, 38 insertions(+), 23 deletions(-)
diff --git a/refs.c b/refs.c
index 3926136..ca854d6 100644
--- a/refs.c
+++ b/refs.c
@@ -589,6 +589,8 @@ static void sort_ref_dir(struct ref_dir *dir)
/* Include broken references in a do_for_each_ref*() iteration: */
#define DO_FOR_EACH_INCLUDE_BROKEN 0x01
+/* Do not recurse into subdirs, just iterate at a single level. */
+#define DO_FOR_EACH_NO_RECURSE 0x02
/*
* Return true iff the reference described by entry can be resolved to
@@ -661,7 +663,8 @@ static int do_one_ref(struct ref_entry *entry, void
*cb_data)
* called for all references, including broken ones.
*/
static int do_for_each_entry_in_dir(struct ref_dir *dir, int offset,
- each_ref_entry_fn fn, void *cb_data)
+ each_ref_entry_fn fn, void *cb_data,
+ int flags)
{
int i;
assert(dir->sorted == dir->nr);
@@ -669,9 +672,13 @@ static int do_for_each_entry_in_dir(struct ref_dir *dir,
int offset,
struct ref_entry *entry = dir->entries[i];
int retval;
if (entry->flag & REF_DIR) {
- struct ref_dir *subdir = get_ref_dir(entry);
- sort_ref_dir(subdir);
- retval = do_for_each_entry_in_dir(subdir, 0, fn,
cb_data);
+ if (flags & DO_FOR_EACH_NO_RECURSE) {
+ struct ref_dir *subdir = get_ref_dir(entry);
+ sort_ref_dir(subdir);
+ retval = do_for_each_entry_in_dir(subdir, 0,
+ fn, cb_data,
+ flags);
+ }
} else {
retval = fn(entry, cb_data);
}
@@ -691,7 +698,8 @@ static int do_for_each_entry_in_dir(struct ref_dir *dir,
int offset,
*/
static int do_for_each_entry_in_dirs(struct ref_dir *dir1,
struct ref_dir *dir2,
-each_ref_entry_fn fn, void *cb_data)
+each_ref_entry_fn fn, void *cb_data,
+int flags)
{
int retval;
int i1 = 0, i2 = 0;
@@ -702,10 +710,12 @@ static int do_for_each_entry_in_dirs(struct ref_dir *dir1,
struct ref_entry *e1, *e2;
int cmp;
if (i1 == dir1->nr) {
- return do_for_each_entry_in_dir(dir2, i2, fn, cb_data);
+ return do_for_each_entry_in_dir(dir2, i2, fn, cb_data,
+ flags);
}
if (i2 == dir2->nr) {
- return do_for_each_entry_in_dir(dir1, i1, fn, cb_data);
+ return do_for_each_entry_in_dir(dir1, i1, fn, cb_data,
+ flags);
}
e1 = dir1->entries[i1];
e2 = dir2->entries[i2];
@@ -713,12 +723,15 @@ static int do_for_each_entry_in_dirs(struct ref_dir *dir1,
if (cmp == 0) {
if ((e1->flag & REF_DIR) && (e2->flag & REF_DIR)) {
/* Both are directories; descend them in
parallel. */
- struct ref_dir *subdir1 = get_ref_dir(e1);
- struct ref_dir *subdir2 = get_ref_dir(e2);
- sort_ref_dir(subdir1);
- sort_ref_dir(subdir2);
- retval = do_for_each_entry_in_dirs(
- subdir1, subdir2, fn, cb_data);
+ if (!(flags & DO_FOR_EACH_NO_RECURSE)) {
+ struct ref_dir *subdir1 =
g
[PATCH v2 1/5] cat-file: refactor error handling of batch_objects
This just pulls the return value for the function out of the
inner loop, so we can break out of the loop rather than do
an early return. This will make it easier to put any cleanup
for the function in one place.
Signed-off-by: Jeff King
---
builtin/cat-file.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f8288c8..971cdde 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -263,6 +263,7 @@ static int batch_objects(struct batch_options *opt)
{
struct strbuf buf = STRBUF_INIT;
struct expand_data data;
+ int retval = 0;
if (!opt->format)
opt->format = "%(objectname) %(objecttype) %(objectsize)";
@@ -294,8 +295,6 @@ static int batch_objects(struct batch_options *opt)
warn_on_object_refname_ambiguity = 0;
while (strbuf_getline(&buf, stdin, '\n') != EOF) {
- int error;
-
if (data.split_on_whitespace) {
/*
* Split at first whitespace, tying off the beginning
@@ -310,12 +309,12 @@ static int batch_objects(struct batch_options *opt)
data.rest = p;
}
- error = batch_one_object(buf.buf, opt, &data);
- if (error)
- return error;
+ retval = batch_one_object(buf.buf, opt, &data);
+ if (retval)
+ break;
}
- return 0;
+ return retval;
}
static const char * const cat_file_usage[] = {
--
1.8.5.2.500.g8060133
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/5] cat-file: fix a minor memory leak in batch_objects
We should always have been freeing our strbuf, but doing so consistently was annoying until the refactoring in the previous patch. Signed-off-by: Jeff King --- builtin/cat-file.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 971cdde..ce79103 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -314,6 +314,7 @@ static int batch_objects(struct batch_options *opt) break; } + strbuf_release(&buf); return retval; } -- 1.8.5.2.500.g8060133 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] speeding up 40-hex ambiguity check
On Tue, Jan 07, 2014 at 05:08:56PM -0500, Jeff King wrote: > > OK. I agree with that line of thinking. Let's take it one step at > > a time, i.e. do c. and also use warn_on_object_refname_ambiguity in > > "rev-list --stdin" first and leave the simplification (i.e. b.) for > > later. > > Here's a series to do that. The first three are just cleanups I noticed > while looking at the problem. > > While I was writing the commit messages, though, I had a thought. Maybe > we could simply do the check faster for the common case that most refs > do not look like object names? Right now we blindly call dwim_ref for > each get_sha1 call, which is the expensive part. If we instead just > loaded all of the refnames from the dwim_ref location (basically heads, > tags and the top-level of "refs/"), we could build an index of all of > the entries matching the 40-hex pattern. In 99% of cases, this would be > zero entries, and the check would collapse to a simple integer > comparison (and even if we did have one, it would be a simple binary > search in memory). That turned out very nicely, and I think we can drop the extra flag entirely. Brodie's patch still makes sense, for people who do want to turn off ambiguity warnings entirely (and I built on his patch, which matters textually for 4 and 5, but it would be easy to rebase). I'm cc-ing Michael, since it is his ref-traversal code I am butchering in the 3rd patch. The first two are the unrelated cleanups from v1. They are not necessary, but I do not see any reason not to include them. [1/5]: cat-file: refactor error handling of batch_objects [2/5]: cat-file: fix a minor memory leak in batch_objects [3/5]: refs: teach for_each_ref a flag to avoid recursion [4/5]: get_sha1: speed up ambiguous 40-hex test [5/5]: get_sha1: drop object/refname ambiguity flag -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: Preferred local submodule branches (was: Introduce git submodule attached update)
On Tue, Jan 07, 2014 at 02:36:25PM -0800, W. Trevor King wrote: > There are three branches that submodule folks usually care about: > > 1. The linked $sha1 in the superproject (set explicitly for every >superproject commit, and thus for every superproject branch). > 2. The remote-tracking submodule..branch that lives in the >upstream submodule..url repository. > 3. The submodule's locally checked out branch, which we currently let >the developer setup by hand, which is used integrated with one of >the other two branches during non-checkout updates. > > Git is currently a bit weak on conveniently handling type-3 branches. > “Just use what the developer has setup” works well for many basic > workflows, but falls short for: > > * Cloning-updates, where we currently always setup a detached HEAD. > * Workflows where the preferred type-3 branch depends on the > superproject branch. > > The former is easy to fix [1] if you accept submodule..branch as > a guess, but this conflates the type-2 and type-3 branches. > > For the latter, you'd want something like: > > On Mon, Jan 06, 2014 at 08:10:04PM -0800, W. Trevor King wrote: > > * Auto checkout of the preferred branch > > * Can do this at clone-update time with my patch. > > * For later submodule branch switches, maybe we want: > > > > git submodule checkout [-b ] […] > > > > Then if a user blows off their detached HEAD, at least they'll > > feel a bit sheepish afterwards. > > which would likely need some of Jens' new core checkout handling [2]. > > [1]: Using something along the lines of my > http://article.gmane.org/gmane.comp.version-control.git/239967 > [2]: http://article.gmane.org/gmane.comp.version-control.git/240117 For example, in Jonathan's recent version of Jens' series, the initial-setup and update functionality are moving into C. See: * populate_submodule() [1] for the initial-clone setup (calling 'read-tree'), and * update_submodule() [2] for subsequent updates (calling 'checkout -q' with an optional '-f') this is where any submodule..local-branch would come into play, if we decide to go down that route. It doesn't look like the C updates have the auto-clone functionality that the Bash updates have. I'm not sure if that's in the pipe or not. I'm not as familiar with the C implementation though, so maybe I'm missing the mark here. Cheers, Trevor [1]: http://article.gmane.org/gmane.comp.version-control.git/239698 [2]: http://article.gmane.org/gmane.comp.version-control.git/239699 -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/2] Introduce git submodule attached update
On Tue, Jan 07, 2014 at 11:51:28PM +0100, Heiko Voigt wrote: > On Mon, Jan 06, 2014 at 08:10:04PM -0800, W. Trevor King wrote: > > Here's an attempted summary of our desires, and my ideal route > > forward: > > > > * Preferred local submodule branches for each superproject branch. > > * Not currently supported by Git. > > * Requires some sort of per-superproject-branch .git/config. > > * Fall back to the remote-tracking submodule..branch? > > > > * Auto checkout of the preferred branch > > * Can do this at clone-update time with my patch. > > * For later submodule branch switches, maybe we want: > > > > git submodule checkout [-b ] […] > > > > Then if a user blows off their detached HEAD, at least they'll > > feel a bit sheepish afterwards. > > Well, for development on a detached HEAD in a submodule we are currently > not very careful anyway. A simple > > git submodule update > > will already blow away any detached HEAD work. Only if you use the checkout strategy. With --merge or --rebase, you'll have the $sha1 (or upstream remote with --remote) integrated with your detached HEAD work. You end up with a new detached HEAD containing the result of the integration (just confirmed with tests using Git v1.8.3.2). That seems reasonable to me, so I'm happy with the integration logic. > But AFAIK it should trigger the "you are leaving commits from a > detached HEAD behind" warning, so there is some safeguard and > recovery. I did not see those in testing with Git v1.8.3.2, likely because of the '-f -q' we pass to 'git checkout' for checkout-mode updates. Regardless of branch integration issues, I think a per-superproject-branch preferred submodule branch is important for 'git checkout' to work in the superproject. If you want: * submodule branch master for superproject branch master, and * submodule branch my-feature for superproject branch my-feature, $ git checkout my-feature in the superproject is currently going to leave you with the submodule on master, which is not convenient ;). I think we should come up with a better solution to the superproject checkout problem before adding in additional complications due to branch integration ;). Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: Re: [PATCH 2/2] Introduce git submodule attached update
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On Mon, Jan 06, 2014 at 08:10:04PM -0800, W. Trevor King wrote: > Here's an attempted summary of our desires, and my ideal route > forward: > > * Preferred local submodule branches for each superproject branch. > * Not currently supported by Git. > * Requires some sort of per-superproject-branch .git/config. > * Fall back to the remote-tracking submodule..branch? > > * Auto checkout of the preferred branch > * Can do this at clone-update time with my patch. > * For later submodule branch switches, maybe we want: > > git submodule checkout [-b ] […] > > Then if a user blows off their detached HEAD, at least they'll > feel a bit sheepish afterwards. Well, for development on a detached HEAD in a submodule we are currently not very careful anyway. A simple git submodule update will already blow away any detached HEAD work. But AFAIK it should trigger the "you are leaving commits from a detached HEAD behind" warning, so there is some safeguard and recovery. Cheers Heiko -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.14 (GNU/Linux) iEYEARECAAYFAlLMhPAACgkQjLR3Aoip+rqP6wCeIhtpWLJC3XVO3nu2ViQTbHPg T5wAoLLEZ256GOOjBxoTKo2/FmfvQGLp =+bqm -END PGP SIGNATURE- -- 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: Re: [RFC v2] submodule: Respect requested branch on all clones
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Hi, here my current thoughts in a kind of summary email. On Tue, Jan 07, 2014 at 11:45:03AM -0800, W. Trevor King wrote: > On Tue, Jan 07, 2014 at 08:19:49PM +0100, Francesco Pretto wrote: > > 2014/1/7 Junio C Hamano : > > > It is not immediately obvious to me why anybody who specifies the > > > submodule.*.branch variable to say "I want _that_ branch" not to > > > want to be on that branch but in a detached state, so from that > > > perspective, submodule.*.attach feels superfluous. > > > > Junio, for what it concerns me I fully support this patch as, IMO, it > > makes cleaner the role of the property "submodule..branch". > > No, submodule..branch is the name of the remote-tracking branch > for 'update --remote'. In this patch, I'm using it as a hint for the > preferred local branch name [1], which I now think was a bad idea. > After [2], I think that we should just define the preferred local > branch name explicitly (submodule..local-branch?). I am not so sure about that. Having an extra value adds more configuration burden to the user and it also does not help to understand how this feature is supposed to be used. Even though I was confused in the first place by the remote/local branch switch for this option, after thinking a little bit more about it I think it makes perfect sense to use the branch option as a hint for the local branch. Let me explain by an example. Suppose we have the following setup: 1. Fast-forward situation superproject submodule master PA--->A master | B origin/master Lets say superproject has submodule.submodule.branch=master and submodule.submodule.update=merge. Doing the initial update which clones results in the submodules master branch being set to the sha1 registered in the superproject. Now an update to the newest master in submodule is straightforward: $ git submodule update --remote 2. Direct work situation The developer start with the same setup as in situation 1 but now directly starts to work in the submodule and creates commit C. superproject submodule master PA--->A |\ origin/master B C master $ git submodule update --remote $ git commit -a -m "update submodule" gets him this: superproject submodule PA--->A ||\ | origin/master B C ||/ master PB--->D master Where now both the submodule and the superproject can be directly pushed. If origin/master in the submodule is tracked by master this is actually one command $ git push --recurse-submodules=on-demand So with your (Trevors) patch and reusing submodule..branch using this kind of direct work in submodules is made easy. And wasn't that what people always requested? ;-) Well, at least if you do not use feature branches this makes it easy. But I think that is a good start make the simple things easy first. Then we can later discuss the more complicated ones. It seems to me that is also the case David wants for his emacs/CEDET workflow: Make it easy for the superproject developers to directly push out trivial fixes to the submodule. And it also seems to me that is want Francesco wants. One thing is missing though (and I think thats where Francesco came from): What if the developer already has a detached HEAD in the submodule? How does he attach to a branch? For this we need something similar to Francescos attach/detach or Trevors submodule checkout with Junio's checkout HEAD~0 from here[1]. I am still undecided how we should call it. Because of my Idea for feature branch support - --- For the branch attaching feature I would also like something that can actually modify .git/config and for me more importantly .gitmodules. So e.g. if I want to work on a longer lived feature branch in a submodule which I need in a feature branch in the superproject I would do something like this: $ git submodule checkout --gitmodules --merge -b hv/my-cool-feature Which should create a local feature branch hv/my-cool-feature in the submodule, checkout that branch and modify .gitmodules (because of --gitmodules) to have submodule..update=merge, submodule..branch=hv/my-cool-feature and stage that to the index. This is a temporary setting so everyone who is working together can update their branches easily. Once finished (with the prove that the big feature in the superproject works) everyone can go and polish the submodule branches, get their changes accepted there first, and then the update/branch setting local for this branch will be dropped. In this workflow these settings never enter a stable branch but are still very useful to transport this information while developing. Just an idea of a future extension we should keep in mind when designing the command to attach to a branch. But maybe the command
Preferred local submodule branches (was: Introduce git submodule attached update)
On Tue, Jan 07, 2014 at 10:51:34PM +0100, Francesco Pretto wrote: > 2014/1/7 W. Trevor King : > > > > I'd be happy to hear ideas about superproject-branch-specific local > > overrides to a hypothetical submodule..local-branch, in the > > event that a developer doesn't like a default set in .gitmodules. If > > I could think of a way to do that, we could avoid this heuristic > > approach, and make the local submodule..local-branch > > vs. remote-tracking submodule..branch distinction more obvious. > > Uh, I think you got it wrong in the other thread: I'm grafting this discussion back on to the thread where I proposed submodule..local-branch. > I didn't proposed such feature. Right. I proposed this feature after reading your proposed workflow. > I just wanted the attached submodule use case to be supported and of > course "--branch means attached" is even easier to get this. As I understood it, the '--branch means attached' stuff was tied up with automatic --remote updates. There are three branches that submodule folks usually care about: 1. The linked $sha1 in the superproject (set explicitly for every superproject commit, and thus for every superproject branch). 2. The remote-tracking submodule..branch that lives in the upstream submodule..url repository. 3. The submodule's locally checked out branch, which we currently let the developer setup by hand, which is used integrated with one of the other two branches during non-checkout updates. Git is currently a bit weak on conveniently handling type-3 branches. “Just use what the developer has setup” works well for many basic workflows, but falls short for: * Cloning-updates, where we currently always setup a detached HEAD. * Workflows where the preferred type-3 branch depends on the superproject branch. The former is easy to fix [1] if you accept submodule..branch as a guess, but this conflates the type-2 and type-3 branches. For the latter, you'd want something like: On Mon, Jan 06, 2014 at 08:10:04PM -0800, W. Trevor King wrote: > * Auto checkout of the preferred branch > * Can do this at clone-update time with my patch. > * For later submodule branch switches, maybe we want: > > git submodule checkout [-b ] […] > > Then if a user blows off their detached HEAD, at least they'll > feel a bit sheepish afterwards. which would likely need some of Jens' new core checkout handling [2]. Cheers, Trevor [1]: Using something along the lines of my http://article.gmane.org/gmane.comp.version-control.git/239967 [2]: http://article.gmane.org/gmane.comp.version-control.git/240117 -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [PATCH v3] stash: handle specifying stashes with spaces
Thomas Rast writes:
> Junio C Hamano writes:
>
>> Øystein Walle writes:
>>
>>> + git stash &&
>>> + test_tick &&
>>> + echo cow > file &&
>>> + git stash &&
>>> + git stash apply "stash@{Thu Apr 7 15:17:13 2005 -0700}" &&
>>
>> This is brittle. If new tests are added before this, the test_tick
>> will give you different timestamp and this test will start failing.
>>
>> Perhaps grab the timestamp out of the stash that was created [...]
>
> Hmm, now that I stare at this, why not simply use something like
>
> git stash apply "stash@{ 0 }"
>
> It seems to refer to the same as stash@{0} as one would expect, while
> still triggering the bug with unpatched git-stash.
Yeah, that is fine as well. For the record, here is what I
tentatively queued.
-- >8 --
From: Øystein Walle
Date: Tue, 7 Jan 2014 09:22:15 +0100
Subject: [PATCH] stash: handle specifying stashes with $IFS
When trying to pop/apply a stash specified with an argument
containing IFS whitespace, git-stash will throw an error:
$ git stash pop 'stash@{two hours ago}'
Too many revisions specified: stash@{two hours ago}
This happens because word splitting is used to count non-option
arguments. Make use of rev-parse's --sq option to quote the arguments
for us to ensure a correct count. Add quotes where necessary.
Also add a test that verifies correct behaviour.
Helped-by: Thomas Rast
Signed-off-by: Øystein Walle
Signed-off-by: Junio C Hamano
---
git-stash.sh | 14 +++---
t/t3903-stash.sh | 12
2 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index 1e541a2..f0a94ab 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -358,7 +358,7 @@ parse_flags_and_rev()
i_tree=
u_tree=
- REV=$(git rev-parse --no-flags --symbolic "$@") || exit 1
+ REV=$(git rev-parse --no-flags --symbolic --sq "$@") || exit 1
FLAGS=
for opt
@@ -376,7 +376,7 @@ parse_flags_and_rev()
esac
done
- set -- $REV
+ eval set -- $REV
case $# in
0)
@@ -391,13 +391,13 @@ parse_flags_and_rev()
;;
esac
- REV=$(git rev-parse --quiet --symbolic --verify $1 2>/dev/null) || {
+ REV=$(git rev-parse --quiet --symbolic --verify "$1" 2>/dev/null) || {
reference="$1"
die "$(eval_gettext "\$reference is not valid reference")"
}
- i_commit=$(git rev-parse --quiet --verify $REV^2 2>/dev/null) &&
- set -- $(git rev-parse $REV $REV^1 $REV: $REV^1: $REV^2: 2>/dev/null) &&
+ i_commit=$(git rev-parse --quiet --verify "$REV^2" 2>/dev/null) &&
+ set -- $(git rev-parse "$REV" "$REV^1" "$REV:" "$REV^1:" "$REV^2:"
2>/dev/null) &&
s=$1 &&
w_commit=$1 &&
b_commit=$2 &&
@@ -408,8 +408,8 @@ parse_flags_and_rev()
test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${REV%@*}")"
&&
IS_STASH_REF=t
- u_commit=$(git rev-parse --quiet --verify $REV^3 2>/dev/null) &&
- u_tree=$(git rev-parse $REV^3: 2>/dev/null)
+ u_commit=$(git rev-parse --quiet --verify "$REV^3" 2>/dev/null) &&
+ u_tree=$(git rev-parse "$REV^3:" 2>/dev/null)
}
is_stash_like()
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index debda7a..5b79b21 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -673,4 +673,16 @@ test_expect_success 'store updates stash ref and reflog' '
grep quux bazzy
'
+test_expect_success 'handle stash specification with spaces' '
+ git stash clear &&
+ echo pig >file &&
+ git stash &&
+ stamp=$(git log -g --format="%cd" -1 refs/stash) &&
+ test_tick &&
+ echo cow >file &&
+ git stash &&
+ git stash apply "stash@{$stamp}" &&
+ grep pig file
+'
+
test_done
--
1.8.5.2-419-g5ca323a
--
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] format-patch: introduce format.defaultTo
Jeff King writes: > And even in a centralized workflow, I see "upstream" creating problems. > E.g., you fork a feature branch in the centralized repo; it should not > get pushed straight back to "master"! And that is why we invented > "simple", to prevent such things. Oh, don't get me wrong. I personally wouldn't imagine forking 'topic' from the shared 'master', call the result perfect and push it directly back to the shared 'master'. But the 'upstream' setting was added exactly to support that. In such a case, I would have 'master' that is forked from the shared 'master', 'topic' that is forked from my 'master', and pushing back would be a two-step process, first updating my 'master' in sync with the shared 'master', merging 'topic' into it to make sure the result is sane and then push it back to the shared 'master'. And in that set-up, 'upstream' would work fine as the upstream of my 'master' is the shared 'master', even though 'current' or even 'matching' would work just as well. So in that sense, I do not see 'upstream' as so broken as you seem to be saying. One gap in that line of thought might be that I am sane enough not to attempt "git push" while I am on my 'topic', 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: [PATCH v3] stash: handle specifying stashes with spaces
Junio C Hamano writes:
> Øystein Walle writes:
>
>> +git stash &&
>> +test_tick &&
>> +echo cow > file &&
>> +git stash &&
>> +git stash apply "stash@{Thu Apr 7 15:17:13 2005 -0700}" &&
>
> This is brittle. If new tests are added before this, the test_tick
> will give you different timestamp and this test will start failing.
>
> Perhaps grab the timestamp out of the stash that was created [...]
Hmm, now that I stare at this, why not simply use something like
git stash apply "stash@{ 0 }"
It seems to refer to the same as stash@{0} as one would expect, while
still triggering the bug with unpatched git-stash.
--
Thomas Rast
t...@thomasrast.ch
--
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] format-patch: introduce format.defaultTo
On Tue, Jan 07, 2014 at 02:06:12PM -0800, Junio C Hamano wrote:
> Jeff King writes:
>
> > I think that is sensible, and only heightens my sense of the "upstream"
> > push.default as useless. :)
>
> Yes, it only is good for centralized world (it was designed back in
> the centralized days after all, wasn't it?).
I do not think there is any "centralized days". From day one, Linus
advocated a triangular workflow, and that is how git and kernel develop
has always been done. And that is why the default of "matching" was
there.
There were people who came later, and who still exist today, who use git
in an SVN-like centralized way. So if there were centralized days, we
are in them now. :)
I just do not see any real advantage even in a centralized world for
"upstream" versus "current". Before remote.pushdefault, I can
potentially see some use (if you want to abuse @{upstream}), but now I
do not see any point.
And even in a centralized workflow, I see "upstream" creating problems.
E.g., you fork a feature branch in the centralized repo; it should not
get pushed straight back to "master"! And that is why we invented
"simple", to prevent such things.
I dunno. I have not gone back and read all of the arguments around
push.default from last year. It is entirely possible everything I just
said was refuted back then, and I am needlessly rehashing old arguments.
I remember that Matthieu was one of the main advocates of "upstream". I
am cc-ing him here to bring his attention (not just to this message, but
to the whole thread).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/4] revision: turn off object/refname ambiguity check for --stdin
We currently check that any 40-hex object name we receive is
not also a refname, and output a warning if this is the
case. When "rev-list --stdin" is used to receive object
names, we may receive a large number of inputs, and the cost
of checking each name as a ref is relatively high.
Commit 25fba78d already dropped this warning for "cat-file
--batch-check". The same reasoning applies for "rev-list
--stdin". Let's disable the check in that instance.
Here are before and after numbers:
$ git rev-list --all >commits
[before]
$ best-of-five -i commits ./git rev-list --stdin --no-walk --pretty=raw
real0m0.675s
user0m0.552s
sys 0m0.120s
[after]
$ best-of-five -i commits ./git rev-list --stdin --no-walk --pretty=raw
real0m0.415s
user0m0.400s
sys 0m0.012s
Signed-off-by: Jeff King
---
Obviously we drop this one (and revert 25fba78d) if I can just make the
check faster.
revision.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/revision.c b/revision.c
index a68fde6..87d04dd 100644
--- a/revision.c
+++ b/revision.c
@@ -1576,7 +1576,9 @@ static void read_revisions_from_stdin(struct rev_info
*revs,
{
struct strbuf sb;
int seen_dashdash = 0;
+ int save_warning = warn_on_object_refname_ambiguity;
+ warn_on_object_refname_ambiguity = 0;
strbuf_init(&sb, 1000);
while (strbuf_getwholeline(&sb, stdin, '\n') != EOF) {
int len = sb.len;
@@ -1595,6 +1597,7 @@ static void read_revisions_from_stdin(struct rev_info
*revs,
REVARG_CANNOT_BE_FILENAME))
die("bad revision '%s'", sb.buf);
}
+ warn_on_object_refname_ambiguity = save_warning;
if (seen_dashdash)
read_pathspec_from_stdin(revs, &sb, prune);
strbuf_release(&sb);
--
1.8.5.2.500.g8060133
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] cat-file: refactor error handling of batch_objects
This just pulls the return value for the function out of the
inner loop, so we can break out of the loop rather than do
an early return. This will make it easier to put any cleanup
for the function in one place.
Signed-off-by: Jeff King
---
Just making the subsequent diffs less noisy...
builtin/cat-file.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f8288c8..971cdde 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -263,6 +263,7 @@ static int batch_objects(struct batch_options *opt)
{
struct strbuf buf = STRBUF_INIT;
struct expand_data data;
+ int retval = 0;
if (!opt->format)
opt->format = "%(objectname) %(objecttype) %(objectsize)";
@@ -294,8 +295,6 @@ static int batch_objects(struct batch_options *opt)
warn_on_object_refname_ambiguity = 0;
while (strbuf_getline(&buf, stdin, '\n') != EOF) {
- int error;
-
if (data.split_on_whitespace) {
/*
* Split at first whitespace, tying off the beginning
@@ -310,12 +309,12 @@ static int batch_objects(struct batch_options *opt)
data.rest = p;
}
- error = batch_one_object(buf.buf, opt, &data);
- if (error)
- return error;
+ retval = batch_one_object(buf.buf, opt, &data);
+ if (retval)
+ break;
}
- return 0;
+ return retval;
}
static const char * const cat_file_usage[] = {
--
1.8.5.2.500.g8060133
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] cat-file: restore ambiguity warning flag in batch_objects
Since commit 25fba78, we turn off the object/refname ambiguity warning using a global flag. However, we never restore it. This doesn't really matter in the current code, since the program generally exits immediately after the function is done, but it's good code hygeine to clean up after ourselves. Signed-off-by: Jeff King --- builtin/cat-file.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index ce79103..c64e287 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -264,6 +264,7 @@ static int batch_objects(struct batch_options *opt) struct strbuf buf = STRBUF_INIT; struct expand_data data; int retval = 0; + int save_warning = warn_on_object_refname_ambiguity; if (!opt->format) opt->format = "%(objectname) %(objecttype) %(objectsize)"; @@ -314,6 +315,7 @@ static int batch_objects(struct batch_options *opt) break; } + warn_on_object_refname_ambiguity = save_warning; strbuf_release(&buf); return retval; } -- 1.8.5.2.500.g8060133 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/4] cat-file: fix a minor memory leak in batch_objects
We should always have been freeing our strbuf, but doing so consistently was annoying until the refactoring in the previous patch. Signed-off-by: Jeff King --- builtin/cat-file.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 971cdde..ce79103 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -314,6 +314,7 @@ static int batch_objects(struct batch_options *opt) break; } + strbuf_release(&buf); return retval; } -- 1.8.5.2.500.g8060133 -- 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] sha1_name: don't resolve refs when core.warnambiguousrefs is false
On Tue, Jan 07, 2014 at 12:31:57PM -0800, Junio C Hamano wrote: > > c. Just leave it at Brodie's patch with nothing else on top. > > > > My thinking in favor of (b) was basically "does anybody actually care > > about ambiguous refs in this situation anyway?". If they do, then I > > think (c) is my preferred choice. > > OK. I agree with that line of thinking. Let's take it one step at > a time, i.e. do c. and also use warn_on_object_refname_ambiguity in > "rev-list --stdin" first and leave the simplification (i.e. b.) for > later. Here's a series to do that. The first three are just cleanups I noticed while looking at the problem. While I was writing the commit messages, though, I had a thought. Maybe we could simply do the check faster for the common case that most refs do not look like object names? Right now we blindly call dwim_ref for each get_sha1 call, which is the expensive part. If we instead just loaded all of the refnames from the dwim_ref location (basically heads, tags and the top-level of "refs/"), we could build an index of all of the entries matching the 40-hex pattern. In 99% of cases, this would be zero entries, and the check would collapse to a simple integer comparison (and even if we did have one, it would be a simple binary search in memory). Our index is more racy than actually checking the filesystem, but I don't think it matters here. Anyway, here is the series I came up with, in the meantime. I can take a quick peek at just making it faster, too. [1/4]: cat-file: refactor error handling of batch_objects [2/4]: cat-file: fix a minor memory leak in batch_objects [3/4]: cat-file: restore ambiguity warning flag in batch_objects [4/4]: revision: turn off object/refname ambiguity check for --stdin -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] format-patch: introduce format.defaultTo
Jeff King writes: > I think that is sensible, and only heightens my sense of the "upstream" > push.default as useless. :) Yes, it only is good for centralized world (it was designed back in the centralized days after all, wasn't 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 v2] submodule: Respect requested branch on all clones
2014/1/7 W. Trevor King : > > I'd be happy to hear ideas about superproject-branch-specific local > overrides to a hypothetical submodule..local-branch, in the > event that a developer doesn't like a default set in .gitmodules. If > I could think of a way to do that, we could avoid this heuristic > approach, and make the local submodule..local-branch > vs. remote-tracking submodule..branch distinction more obvious. > Uh, I think you got it wrong in the other thread: I didn't proposed such feature. I just wanted the attached submodule use case to be supported and of course "--branch means attached" is even easier to get this. -- 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] Fix safe_create_leading_directories() for Windows
On Tue, Jan 7, 2014 at 6:56 PM, Johannes Schindelin wrote: >> > Well, you and I both know how easy GitHub's pull request made things >> > for us as well as for contributors. I really cannot thank Erik enough >> > for bullying me into using and accepting them. >> >> Huh? I don't think you refer to me, because I really dislike them (and I >> always have IIRC). > > Ah yes, I misremembered. You were actually opposed to using them and I > thought we should be pragmatic to encourage contributions. Not that it matters too much, but I guess it was me who talked Dscho into moving to GitHub and using / accepting pull requests :-) > In any case, I do think that the contributions we got via pull requests > were in general contributions we would not otherwise have gotten. I absolutely think so, too. -- Sebastian Schuberth -- 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] format-patch: introduce branch.*.forkedFrom
Jeff King wrote:
> I definitely respect the desire to reuse the existing tooling we have
> for @{u}. At the same time, I think you are warping the meaning of
> @{u} somewhat. It is _not_ your upstream here, but rather another
> version of the branch that has useful changes in it. That might be
> splitting hairs a bit, but I think you will find that the differences
> leak through in inconvenient spots (like format-patch, where you really
> _do_ want to default to the true upstream).
Thanks for the clear reasoning.
> If we add "@{publish}" (and "@{pu}"), then it becomes very convenient to
> refer to the ram/ version of your branch. That seems like an obvious
> first step to me. We don't have to add new config, because
> "branch.*.pushremote" already handles this.
Agreed. I'll start working on @{publish}. It's going to take quite a
bit of effort, because I won't actually start using it until my prompt
is @{publish}-aware.
> Now you can do "git rebase @{pu}" which is nice, but not _quite_ as nice
> as "git rebase", which defaults to "@{u}". That first step might be
> enough, and I'd hold off there and try it out for a few days or weeks
> first. But if you find in your workflow that you are having to specify
> "@{pu}" a lot, then maybe it is worth adding an option to default rebase
> to "@{pu}" instead of "@{u}".
Actually, I'm not sure I'd use "git rebase @{pu}"; for me @{pu} is
mainly a source of information for taking apart to build a new series.
--
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 v2] submodule: Respect requested branch on all clones
On Tue, Jan 07, 2014 at 09:09:19PM +0100, Francesco Pretto wrote:
> 2014/1/7 W. Trevor King :
> >> Trevor, maybe it was not clear. But I wanted to say:
> >>
> >> " I fully support *Trevor's* patch..." :)
> >
> > Which I appreciate ;). I still though I should point out that my
> > patch *confuses* the role of submodule..branch :p.
>
> You are welcome. Also, at your wish, can you please reply also in
> public?
Here you go.
I'd be happy to hear ideas about superproject-branch-specific local
overrides to a hypothetical submodule..local-branch, in the
event that a developer doesn't like a default set in .gitmodules. If
I could think of a way to do that, we could avoid this heuristic
approach, and make the local submodule..local-branch
vs. remote-tracking submodule..branch distinction more obvious.
It would also be nice if submodule..branch was just an initial
setup-time and detached-HEAD default. If the submodule is on a branch
it would make more sense to use the checked-out branch's @{upstream}.
Cheers,
Trevor
--
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy
signature.asc
Description: OpenPGP digital signature
[PATCH v2 2/2] rm: better document side effects when removing a submodule
The "Submodules" section of the "git rm" documentation mentions what will happen when a submodule with a gitfile gets removed with newer git. But it doesn't talk about what happens when the user changes between commits before and after the removal, which does not remove the submodule from the work tree like using the rm command did the first time. Explain what happens and what the user has to do manually to fix that in the new BUGS section. Also document this behavior in a new test. Signed-off-by: Jens Lehmann --- Documentation/git-rm.txt | 9 + t/t3600-rm.sh| 16 2 files changed, 25 insertions(+) diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt index 9d731b4..f1efc11 100644 --- a/Documentation/git-rm.txt +++ b/Documentation/git-rm.txt @@ -170,6 +170,15 @@ of files and subdirectories under the `Documentation/` directory. (i.e. you are listing the files explicitly), it does not remove `subdir/git-foo.sh`. +BUGS + +Each time a superproject update removes a populated submodule +(e.g. when switching between commits before and after the removal) a +stale submodule checkout will remain in the old location. Removing the +old directory is only safe when it uses a gitfile, as otherwise the +history of the submodule will be deleted too. This step will be +obsolete when recursive submodule update has been implemented. + SEE ALSO linkgit:git-add[1] diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index 540c49b..3d30581 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -705,6 +705,22 @@ test_expect_success 'rm of a populated nested submodule with a nested .git direc rm -rf submod ' +test_expect_success 'checking out a commit after submodule removal needs manual updates' ' + git commit -m "submodule removal" submod && + git checkout HEAD^ && + git submodule update && + git checkout -q HEAD^ 2>actual && + git checkout -q master 2>actual && + echo "warning: unable to rmdir submod: Directory not empty" >expected && + test_i18ncmp expected actual && + git status -s submod >actual && + echo "?? submod/" >expected && + test_cmp expected actual && + rm -rf submod && + git status -s -uno --ignore-submodules=none > actual && + ! test -s actual +' + test_expect_success 'rm of d/f when d has become a non-directory' ' rm -rf d && mkdir d && -- 1.8.5.2.231.gfc86eb1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] mv: better document side effects when moving a submodule
The "Submodules" section of the "git mv" documentation mentions what will happen when a submodule with a gitfile gets moved with newer git. But it doesn't talk about what happens when the user changes between commits before and after the move, which does not update the work tree like using the mv command did the first time. Explain what happens and what the user has to do manually to fix that in the new BUGS section. Also document this behavior in a new test. Reported-by: George Papanikolaou Signed-off-by: Jens Lehmann --- Documentation/git-mv.txt | 12 t/t7001-mv.sh| 21 + 2 files changed, 33 insertions(+) diff --git a/Documentation/git-mv.txt b/Documentation/git-mv.txt index b1f7988..e453132 100644 --- a/Documentation/git-mv.txt +++ b/Documentation/git-mv.txt @@ -52,6 +52,18 @@ core.worktree setting to make the submodule work in the new location. It also will attempt to update the submodule..path setting in the linkgit:gitmodules[5] file and stage that file (unless -n is used). +BUGS + +Each time a superproject update moves a populated submodule (e.g. when +switching between commits before and after the move) a stale submodule +checkout will remain in the old location and an empty directory will +appear in the new location. To populate the submodule again in the new +location the user will have to run "git submodule update" +afterwards. Removing the old directory is only safe when it uses a +gitfile, as otherwise the history of the submodule will be deleted +too. Both steps will be obsolete when recursive submodule update has +been implemented. + GIT --- Part of the linkgit:git[1] suite diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh index 3bfdfed..e3c8c2c 100755 --- a/t/t7001-mv.sh +++ b/t/t7001-mv.sh @@ -442,4 +442,25 @@ test_expect_success 'mv --dry-run does not touch the submodule or .gitmodules' ' git diff-files --quiet -- sub .gitmodules ' +test_expect_success 'checking out a commit before submodule moved needs manual updates' ' + git mv sub sub2 && + git commit -m "moved sub to sub2" && + git checkout -q HEAD^ 2>actual && + echo "warning: unable to rmdir sub2: Directory not empty" >expected && + test_i18ncmp expected actual && + git status -s sub2 >actual && + echo "?? sub2/" >expected && + test_cmp expected actual && + ! test -f sub/.git && + test -f sub2/.git && + git submodule update && + test -f sub/.git && + rm -rf sub2 && + git diff-index --exit-code HEAD && + git update-index --refresh && + git diff-files --quiet -- sub .gitmodules && + git status -s sub2 >actual && + ! test -s actual +' + test_done -- 1.8.5.2.231.gfc86eb1 -- 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] format-patch: introduce format.defaultTo
On Wed, Jan 08, 2014 at 02:55:04AM +0530, Ramkumar Ramachandra wrote:
> Jeff King wrote:
> > My daily procedure is something like:
> >
> > all_topics |
> > while read topic; do "echo $topic $(git rev-parse $topic@{u})"; done |
> > topo_sort |
> > while read topic upstream; do
> > git rebase $upstream $topic || exit 1
> > done
>
> Ah, I was perhaps over-specializing for my own usecase, where
> everything is based off 'master'. I never considered 'master' a "true
> upstream" because I throw away topic branches after the maintainer
> merges them. If you have long-running branches that you work on a
> daily basis, the issue is somewhat different.
What I do is maybe somewhat gross, but I continually rebase my patches
forward as master develops. So they diverge from where Junio has forked
them upstream (which does not necessarily have any relationship with
where I forked from, anyway). The nice thing about this is that
eventually the topic becomes empty, as rebase drops patches that were
merged upstream (or resolve conflicts to end up at an empty patch).
It's a nice way of tracking the progress of the patch upstream, and it
catches any differences between what's upstream and what's in the topic
(in both directions: you see where the maintainer may have marked up
your patch, and you may see a place where you added something to be
squashed but the maintainer missed it). The downside is that sometimes
the conflicts are annoying and complicated (e.g., several patches that
touch the same spot are a pain to rebase on top of themselves; the early
ones are confused that the later changes are already in place).
> My primary concern is that the proposed @{publish} should be a
> first-class citizen; if it has everything that @{u} has, then we're
> both good: you'd primarily use @{u}, while I'd primarily use
> @{publish}.
Definitely. I think that's the world we want to work towards.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/2] better document side effects when [re]moving a submodule
Am 07.01.2014 18:57, schrieb Jens Lehmann: > Am 06.01.2014 23:40, schrieb Junio C Hamano: >> Jens Lehmann writes: >>> Does this new paragraph make it clearer? >> >> Don't we have bugs section that we can use to list the known >> limitations like this? > > Right, will change accordingly in v2. Changes from v1: - Document side effects under the BUGS section - Add similar documentation for "git rm" Jens Lehmann (2): mv: better document side effects when moving a submodule rm: better document side effects when removing a submodule Documentation/git-mv.txt | 12 Documentation/git-rm.txt | 9 + t/t3600-rm.sh| 16 t/t7001-mv.sh| 21 + 4 files changed, 58 insertions(+) -- 1.8.5.2.231.gfc86eb1 -- 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] format-patch: introduce format.defaultTo
Jeff King wrote:
> My daily procedure is something like:
>
> all_topics |
> while read topic; do "echo $topic $(git rev-parse $topic@{u})"; done |
> topo_sort |
> while read topic upstream; do
> git rebase $upstream $topic || exit 1
> done
Ah, I was perhaps over-specializing for my own usecase, where
everything is based off 'master'. I never considered 'master' a "true
upstream" because I throw away topic branches after the maintainer
merges them. If you have long-running branches that you work on a
daily basis, the issue is somewhat different.
>> While doing respins, the prompt doesn't aid you in any way. Besides,
>> on several occasions, I found myself working on the same forked-branch
>> from two different machines; then the publish-point isn't necessarily
>> always a publish-point: it's just another "upstream" for the branch.
>
> Right, things get trickier then. But I don't think there is an automatic
> way around that. Sometimes the published one is more up to date, and
> sometimes the upstream thing is more up to date. You have to manually
> tell git which you are currently basing your work on. I find in such a
> situation that it tends to resolve itself quickly, though, as the first
> step is to pull in the changes you pushed up from the other machine
> anyway (either via "git reset" or "git rebase").
My primary concern is that the proposed @{publish} should be a
first-class citizen; if it has everything that @{u} has, then we're
both good: you'd primarily use @{u}, while I'd primarily use
@{publish}.
--
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] format-patch: introduce format.defaultTo
On Tue, Jan 07, 2014 at 01:07:08PM -0800, Junio C Hamano wrote: > Jeff King writes: > > > Yes, "pushbranch" is probably a better name for what I am referring to. > > I agree that pushremote is probably enough for sane cases. I seem to > > recall that people advocating the "upstream" push-default thought that > > branch name mapping was a useful feature, but I might be > > mis-remembering. I will let those people speak up for the feature if > > they see fit; it seems somewhat crazy to me. > > I think "branch mapping" you recall are for those who want to push > their 'topic' to 'review/topic' or something like that. With Git > post 7cdebd8a (Merge branch 'jc/push-refmap', 2013-12-27), I think > "remote.*.push" can be used to implement that, by the way. Hmm. The top patch of that series still relies on "upstream" being a push destination, though. So if I have a triangular workflow where I fork "topic" from "origin/master", my "git push origin topic" will go to "refs/heads/master" on "origin" under the "upstream" rule. So that seems broken as ever. :) But I guess what you are referring to is that in a triangular world, the second patch lets me do: git config push.default current git config remote.origin.push 'refs/heads/*:refs/review/*' And then "git push", "git push origin", or "git push origin topic" all put it in "review/topic", which is what I want. I think that is sensible, and only heightens my sense of the "upstream" push.default as useless. :) -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] format-patch: introduce branch.*.forkedFrom
On Wed, Jan 08, 2014 at 02:32:10AM +0530, Ramkumar Ramachandra wrote:
> > we should leave @{upstream} as (1), and add a new option to represent
> > (2). Not the other way around.
>
> I have a local branch 'forkedfrom' that has two "sources": 'master'
> and 'ram/forkedfrom'. 'ram/forkedfrom' isn't a "dumb" publish-point:
> the relationship information I get between 'forkedfrom' and
> 'ram/forkedfrom' is very useful; it's what helps me tell how my
> re-roll is doing with respect to the original series; I'd often want
> to cherry-pick commits/ messages from my original series to prepare
> the re-roll, so interaction with this source is quite high. On the
> other hand, the relationship information I get between 'forkedfrom'
> and 'master' is practically useless: 'forkedfrom' is always ahead of
> 'master', and a divergence indicates that I need to rebase; I'll never
> really need to interact with this source.
Thanks for a concrete example.
I definitely respect the desire to reuse the existing tooling we have
for @{u}. At the same time, I think you are warping the meaning of
@{u} somewhat. It is _not_ your upstream here, but rather another
version of the branch that has useful changes in it. That might be
splitting hairs a bit, but I think you will find that the differences
leak through in inconvenient spots (like format-patch, where you really
_do_ want to default to the true upstream).
If we add "@{publish}" (and "@{pu}"), then it becomes very convenient to
refer to the ram/ version of your branch. That seems like an obvious
first step to me. We don't have to add new config, because
"branch.*.pushremote" already handles this.
Now you can do "git rebase @{pu}" which is nice, but not _quite_ as nice
as "git rebase", which defaults to "@{u}". That first step might be
enough, and I'd hold off there and try it out for a few days or weeks
first. But if you find in your workflow that you are having to specify
"@{pu}" a lot, then maybe it is worth adding an option to default rebase
to "@{pu}" instead of "@{u}".
You end up in the same place ("git rebase" without options does what you
want), but I think the underlying data more accurately represents what
is going on (and there is no need to teach "format-patch" anything
special).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] format-patch: introduce format.defaultTo
Jeff King writes:
> Yes, "pushbranch" is probably a better name for what I am referring to.
> I agree that pushremote is probably enough for sane cases. I seem to
> recall that people advocating the "upstream" push-default thought that
> branch name mapping was a useful feature, but I might be
> mis-remembering. I will let those people speak up for the feature if
> they see fit; it seems somewhat crazy to me.
I think "branch mapping" you recall are for those who want to push
their 'topic' to 'review/topic' or something like that. With Git
post 7cdebd8a (Merge branch 'jc/push-refmap', 2013-12-27), I think
"remote.*.push" can be used to implement that, by the way.
>> Frankly, I don't use full triangular workflows myself mainly because
>> my prompt is compromised: when I have a branch.*.remote different from
>> branch.*.pushremote, I'd like to see where my branch is with respect
>> to @{u} and @{publish} (not yet invented);
>
> Yes, as two separate relationships, you would theoretically want to be
> able to see them separately (or simultaneously side by side). Whether
> exposing that in the prompt is too clunky, I don't know (I don't even
> show ahead/behind in my prompt, but rather prefer to query it when I
> care; I have a separate script that queries the ahead/behind against my
> publishing point, but it would be nice if git handled this itself).
Same here. I do not bother a/b in prompt and comparison with
publishing point is done with a custom script. It would be nice to
have it natively, and @{publish} would be a good first step to do
so.
--
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] format-patch: introduce format.defaultTo
On Tue, Jan 07, 2014 at 04:17:00AM +0530, Ramkumar Ramachandra wrote:
> Junio C Hamano wrote:.
> > As I said in the different subthread, I am not convinced that you
> > would need the complexity of branch.*.forkedFrom. If you set your
> > "upstream" to the true upstream (not your publishing point), and
> > have "remote.pushdefault"set to 'publish', you can expect
> >
> > git push
> >
> > to do the right thing, and then always say
> >
> > git show-branch publish/topic topic
>
> I think it's highly sub-optimal to have a local-branch @{u} for
> several reasons; the prompt is almost useless in this case, and it
> will always show your forked-branch ahead of 'master' (assuming that
> 'master' doesn't update itself in the duration of your development).
I actually use local-branch @{u} all the time to represent inter-topic
dependencies. For example, imagine I have topic "bar" which builds on
topic "foo", which is based on master. I would have:
[branch "foo"]
remote = origin
merge = refs/heads/master
[branch "bar"]
remote = .
merge = refs/heads/foo
When I rebase "foo", I want to rebase it against upstream's master. When
I rebase "bar", I want to rebase it against foo. And naturally, upstream
does not necessarily have a "foo", because it is my topic, not theirs (I
_may_ have published my "foo" somewhere, but that is orthogonal, and
anyway my local "foo" is the most up-to-date source, not the pushed
version).
As an aside, if you want to rebase both branches, you have to topo-sort
them to make sure you do "foo" first, then rebase "bar" on the result.
My daily procedure is something like:
all_topics |
while read topic; do "echo $topic $(git rev-parse $topic@{u})"; done |
topo_sort |
while read topic upstream; do
git rebase $upstream $topic || exit 1
done
> While doing respins, the prompt doesn't aid you in any way. Besides,
> on several occasions, I found myself working on the same forked-branch
> from two different machines; then the publish-point isn't necessarily
> always a publish-point: it's just another "upstream" for the branch.
Right, things get trickier then. But I don't think there is an automatic
way around that. Sometimes the published one is more up to date, and
sometimes the upstream thing is more up to date. You have to manually
tell git which you are currently basing your work on. I find in such a
situation that it tends to resolve itself quickly, though, as the first
step is to pull in the changes you pushed up from the other machine
anyway (either via "git reset" or "git rebase").
-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] format-patch: introduce branch.*.forkedFrom
Jeff King wrote:
> I have not carefully read some of the later bits of the discussion from
> last night / this morning, so maybe I am missing something, but this
> seems backwards to me from what Junio and I were discussing earlier.
>
> The point was that the meaning of "@{upstream}" (and "branch.*.merge")
> is _already_ "forked-from", and "push -u" and "push.default=upstream"
> are the odd men out. If we are going to add an option to distinguish the
> two branch relationships:
>
> 1. Where you forked from
>
> 2. Where you push to
>
> we should leave @{upstream} as (1), and add a new option to represent
> (2). Not the other way around.
I have a local branch 'forkedfrom' that has two "sources": 'master'
and 'ram/forkedfrom'. 'ram/forkedfrom' isn't a "dumb" publish-point:
the relationship information I get between 'forkedfrom' and
'ram/forkedfrom' is very useful; it's what helps me tell how my
re-roll is doing with respect to the original series; I'd often want
to cherry-pick commits/ messages from my original series to prepare
the re-roll, so interaction with this source is quite high. On the
other hand, the relationship information I get between 'forkedfrom'
and 'master' is practically useless: 'forkedfrom' is always ahead of
'master', and a divergence indicates that I need to rebase; I'll never
really need to interact with this source.
I'm only thinking in terms of what infrastructure we've already built:
if @{u} is set to 'ram/forkedfrom', we get a lot of information for
free _now_. If @{u} is set to 'master', the current git-status is
unhelpful.
--
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] format-patch: introduce format.defaultTo
On Tue, Jan 07, 2014 at 03:40:56AM +0530, Ramkumar Ramachandra wrote:
> Jeff King wrote:
> > Yeah, I had similar thoughts. I personally use "branch.*.merge" as
> > "forkedFrom", and it seems like we are going that way anyway with things
> > like "git rebase" and "git merge" defaulting to upstream.
>
> My issue with that is that I no idea where my branch is with respect
> to my forked upstream; I find that extremely useful when doing
> re-spins.
Right. I think there are two separate relationships, and they are both
shoe-horned into "upstream". The solution is to let them be configured
separately (and fallback on each other as appropriate to make the burden
less on the user).
> push.default = upstream is a bit of a disaster, in my opinion. I've
> advocated push.default = current on multiple occasions, and wrote the
> initial remote.pushDefault series with that configuration in mind.
Yeah, I agree with all of that.
> > I wonder if it is too late to try to clarify this dual usage. It kind of
> > seems like the push config is "this is the place I publish to". Which,
> > in many workflows, just so happens to be the exact same as the place you
> > forked from. Could we introduce a new branch.*.pushupstream variable
> > that falls back to branch.*.merge? Or is that just throwing more fuel on
> > the fire (more sand in the pit in my analogy, I guess).
>
> We already have a branch.*.pushremote, and I don't see the value of
> branch.*.pushbranch (what you're referring to as pushupstream, I
> assume) except for Gerrit users.
Yes, "pushbranch" is probably a better name for what I am referring to.
I agree that pushremote is probably enough for sane cases. I seem to
recall that people advocating the "upstream" push-default thought that
branch name mapping was a useful feature, but I might be
mis-remembering. I will let those people speak up for the feature if
they see fit; it seems somewhat crazy to me.
> Frankly, I don't use full triangular workflows myself mainly because
> my prompt is compromised: when I have a branch.*.remote different from
> branch.*.pushremote, I'd like to see where my branch is with respect
> to @{u} and @{publish} (not yet invented);
Yes, as two separate relationships, you would theoretically want to be
able to see them separately (or simultaneously side by side). Whether
exposing that in the prompt is too clunky, I don't know (I don't even
show ahead/behind in my prompt, but rather prefer to query it when I
care; I have a separate script that queries the ahead/behind against my
publishing point, but it would be nice if git handled this itself).
> > I admit I haven't thought it through yet, though. And even if it does
> > work, it may throw a slight monkey wrench in the proposed push.default
> > transition.
>
> We're transitioning to push.default = simple which is even simpler
> than current.
Simpler in the sense that it is less likely to do something unexpected.
But the rules are actually more complicated. Two examples:
1. Imagine I make a feature branch "foo" forked off of origin/master, then
"git push" with no arguments. The "current" scheme would go to
"foo" on origin, but "upstream" would go to "master". Since they
don't agree, "simple" will punt and tell me to be more specific.
2. Imagine I have set my default push remote to "publish", am on
master (forked from "origin/master") and I run "git push" without
arguments. "current" would push to "master" on "publish". But
"upstream" will complain, because we are not pushing to our
upstream remote. I believe "simple" will therefore reject this.
In both cases, I think "current" does the sane thing, and "simple" makes
things more complicated. The one saving grace it has is that it punts on
these cases rather than potentially doing something destructive that the
user did not intend.
-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] format-patch: introduce branch.*.forkedFrom
Jeff King writes:
> The point was that the meaning of "@{upstream}" (and "branch.*.merge")
> is _already_ "forked-from", and "push -u" and "push.default=upstream"
> are the odd men out. If we are going to add an option to distinguish the
> two branch relationships:
>
> 1. Where you forked from
>
> 2. Where you push to
>
> we should leave @{upstream} as (1), and add a new option to represent
> (2). Not the other way around.
That matches my feeling as well.
I am not sure if "push -u" is truly odd man out, though. It was an
invention back in the "you fetch from and push to the same place and
there is no other workflow support" days, and in that context, the
"upstream" meant just that: the place you fetch from, which happens
to be the same as where you are pushing to right now. If "push -u"
suddenly stopped setting the configuration to merge back from where
it is pushing, that would regress for centralized folks, so I am not
sure how it could be extended to also support triangular folks, but
I do think @{upstream} should mean "this is where I sync with to
stay abreast with others".
--
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] format-patch: introduce branch.*.forkedFrom
Ramkumar Ramachandra writes:
> Junio C Hamano wrote:
>> I do not mind allowing laziness by defaulting to something, but I am
>> not enthused by an approach that adds the new variable whose value
>> is questionable. The description does not justify at all why
>> @{upstream} is not a good default (unless the workflow is screwed up
>> and @{upstream} is set to point at somewhere that is _not_ a true
>> upstream, that is).
>
> Did you find the explanation I gave in
> http://article.gmane.org/gmane.comp.version-control.git/240077
> reasonable?
No.
--
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] format-patch: introduce branch.*.forkedFrom
Junio C Hamano wrote:
> I do not mind allowing laziness by defaulting to something, but I am
> not enthused by an approach that adds the new variable whose value
> is questionable. The description does not justify at all why
> @{upstream} is not a good default (unless the workflow is screwed up
> and @{upstream} is set to point at somewhere that is _not_ a true
> upstream, that is).
Did you find the explanation I gave in
http://article.gmane.org/gmane.comp.version-control.git/240077
reasonable? I don't know why label the respin-workflow as being
"screwed up".
--
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] format-patch: introduce branch.*.forkedFrom
On Wed, Jan 08, 2014 at 02:00:44AM +0530, Ramkumar Ramachandra wrote:
> On Wed, Jan 8, 2014 at 1:59 AM, Ramkumar Ramachandra
> wrote:
> > A very common workflow for preparing patches involves working off a
> > topic branch and generating patches against 'master' to send off to the
> > maintainer. However, a plain
> >
> > $ git format-patch -o outgoing
> >
> > is a no-op on a topic branch, and the user has to remember to specify
> > 'master' explicitly everytime. This problem is not unique to
> > format-patch; even a
> >
> > $ git rebase -i
> >
> > is a no-op because the branch to rebase against isn't specified.
> >
> > To tackle this problem, introduce branch.*.forkedFrom which can specify
> > the parent branch of a topic branch. Future patches will build
> > functionality around this new configuration variable.
> >
> > Cc: Jeff King
> > Cc: Junio C Hamano
> > Signed-off-by: Ramkumar Ramachandra
I have not carefully read some of the later bits of the discussion from
last night / this morning, so maybe I am missing something, but this
seems backwards to me from what Junio and I were discussing earlier.
The point was that the meaning of "@{upstream}" (and "branch.*.merge")
is _already_ "forked-from", and "push -u" and "push.default=upstream"
are the odd men out. If we are going to add an option to distinguish the
two branch relationships:
1. Where you forked from
2. Where you push to
we should leave @{upstream} as (1), and add a new option to represent
(2). Not the other way around.
Am I missing something?
-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] format-patch: introduce branch.*.forkedFrom
Ramkumar Ramachandra writes:
> A very common workflow for preparing patches involves working off a
> topic branch and generating patches against 'master' to send off to the
> maintainer. However, a plain
>
> $ git format-patch -o outgoing
>
> is a no-op on a topic branch, and the user has to remember to specify
> 'master' explicitly everytime. This problem is not unique to
> format-patch; even a
>
> $ git rebase -i
>
> is a no-op because the branch to rebase against isn't specified.
>
> To tackle this problem, introduce branch.*.forkedFrom which can specify
> the parent branch of a topic branch. Future patches will build
> functionality around this new configuration variable.
>
I do not mind allowing laziness by defaulting to something, but I am
not enthused by an approach that adds the new variable whose value
is questionable. The description does not justify at all why
@{upstream} is not a good default (unless the workflow is screwed up
and @{upstream} is set to point at somewhere that is _not_ a true
upstream, that is).
> Cc: Jeff King
> Cc: Junio C Hamano
> Signed-off-by: Ramkumar Ramachandra
> ---
> Since -M, -C, -D are left in the argc, checking argc < 2 isn't
> sufficient.
>
> I wanted to get an early reaction before wiring up checkout and
> rebase.
>
> But I wanted to discuss the overall idea of the patch.
> builtin/log.c | 21 +
> t/t4014-format-patch.sh | 20
> 2 files changed, 41 insertions(+)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index b97373d..525e696 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -674,6 +674,7 @@ static int thread;
> static int do_signoff;
> static const char *signature = git_version_string;
> static int config_cover_letter;
> +static const char *config_base_branch;
>
> enum {
> COVER_UNSET,
> @@ -750,6 +751,22 @@ static int git_format_config(const char *var, const char
> *value, void *cb)
> config_cover_letter = git_config_bool(var, value) ? COVER_ON :
> COVER_OFF;
> return 0;
> }
> + if (starts_with(var, "branch.")) {
> + const char *name = var + 7;
> + const char *subkey = strrchr(name, '.');
> + struct strbuf buf = STRBUF_INIT;
> +
> + if (!subkey)
> + return 0;
> + strbuf_add(&buf, name, subkey - name);
> + if (branch_get(buf.buf) != branch_get(NULL))
> + return 0;
> + strbuf_release(&buf);
> + if (!strcmp(subkey, ".forkedfrom")) {
> + if (git_config_string(&config_base_branch, var, value))
> + return -1;
> + }
> + }
>
> return git_log_config(var, value, cb);
> }
> @@ -1324,6 +1341,10 @@ int cmd_format_patch(int argc, const char **argv,
> const char *prefix)
> die (_("--subject-prefix and -k are mutually exclusive."));
> rev.preserve_subject = keep_subject;
>
> + if (argc < 2 && config_base_branch) {
> + argv[1] = config_base_branch;
> + argc++;
> + }
> argc = setup_revisions(argc, argv, &rev, &s_r_opt);
> if (argc > 1)
> die (_("unrecognized argument: %s"), argv[1]);
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 73194b2..2ea94af 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1370,4 +1370,24 @@ test_expect_success 'cover letter auto user override' '
> test_line_count = 2 list
> '
>
> +test_expect_success 'branch.*.forkedFrom matches' '
> + mkdir -p tmp &&
> + test_when_finished "rm -rf tmp;
> + git config --unset branch.rebuild-1.forkedFrom" &&
> +
> + git config branch.rebuild-1.forkedFrom master &&
> + git format-patch -o tmp >list &&
> + test_line_count = 2 list
> +'
> +
> +test_expect_success 'branch.*.forkedFrom does not match' '
> + mkdir -p tmp &&
> + test_when_finished "rm -rf tmp;
> + git config --unset branch.foo.forkedFrom" &&
> +
> + git config branch.foo.forkedFrom master &&
> + git format-patch -o tmp >list &&
> + test_line_count = 0 list
> +'
> +
> test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_name: don't resolve refs when core.warnambiguousrefs is false
Jeff King writes: > On Tue, Jan 07, 2014 at 11:38:15AM -0800, Junio C Hamano wrote: > >> >> > Alternatively, I guess "cat-file >> >> > --batch" could just turn off warn_ambiguous_refs itself. >> >> >> >> Sounds like a sensible way to go, perhaps on top of this change? >> > >> > The downside is that we would not warn about ambiguous refs anymore, >> > even if the user was expecting it to. I don't know if that matters much. >> >> That is true already with or without Brodie's change, isn't it? >> With warn_on_object_refname_ambiguity, "cat-file --batch" makes us >> ignore core.warnambigousrefs setting. If we redo 25fba78d >> (cat-file: disable object/refname ambiguity check for batch mode, >> 2013-07-12) to unconditionally disable warn_ambiguous_refs in >> "cat-file --batch" and get rid of warn_on_object_refname_ambiguity, >> the end result would be the same, no? > > No, I don't think the end effect is the same (or maybe we are not > talking about the same thing. :) ). > > There are two ambiguity situations: > > 1. Ambiguous non-fully-qualified refs (e.g., same tag and head name). > > 2. 40-hex sha1 object names which might also be unqualified ref names. > > Prior to 25ffba78d, cat-file checked both (like all the rest of git). > But checking (2) is very expensive,... Ahh, of course. Sorry for forgetting about 1. > The two options I was musing over earlier today were (all on top of > Brodie's patch): > > a. Revert 25ffba78d. With Brodie's patch, core.warnAmbiguousRefs > disables _both_ warnings. So we default to safe-but-slow, but > people who care about performance can turn off ambiguity warnings. > The downside is that you have to know to turn it off manually (and > it's a global config flag, so you end up turning it off > _everywhere_, not just in big queries where it matters). Or "git -c core.warnambiguousrefs=false cat-file --batch", but I think a more important point is that it is no longer automatic for known-to-be-heavy operations, and I agree with you that it is a downside. > b. Revert 25ffba78d, but then on top of it just turn off > warn_ambiguous_refs unconditionally in "cat-file --batch-check". > The downside is that we drop the safety from (1). The upside is > that the code is a little simpler, as we drop the extra flag. > > And obviously: > > c. Just leave it at Brodie's patch with nothing else on top. > > My thinking in favor of (b) was basically "does anybody actually care > about ambiguous refs in this situation anyway?". If they do, then I > think (c) is my preferred choice. OK. I agree with that line of thinking. Let's take it one step at a time, i.e. do c. and also use warn_on_object_refname_ambiguity in "rev-list --stdin" first and leave the simplification (i.e. b.) for later. >> > I kind of feel in the --batch situation that it is somewhat useless (I >> > wonder if "rev-list --stdin" should turn it off, too). >> >> I think doing the same as "cat-file --batch" in "rev-list --stdin" >> makes sense. Both interfaces are designed to grok extended SHA-1s, >> and full 40-hex object names could be ambiguous and we are missing >> the warning for them. > > I'm not sure I understand what you are saying. We _do_ have the warning > for "rev-list --stdin" currently. We do _not_ have the warning for > "cat-file --batch", since my 25ffba78d. What I wanted to say was that we would be discarding the safety for "rev-list --stdin" with the same argument as we did for "cat-file --batch". If the argument for the earlier "cat-file --batch" were "this interface only takes raw 40-hex object names", then the situation would have been different, but that is not the case. > I was wondering if rev-list should go the same way as 25ffba78d, > for efficiency reasons (e.g., think piping to "rev-list --no-walk > --stdin"). Yes, and I was trying to agree with that, but apparently I failed ;-) -- 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] format-patch: introduce branch.*.forkedFrom
[Fixed typo in Junio's address]
On Wed, Jan 8, 2014 at 1:59 AM, Ramkumar Ramachandra wrote:
> A very common workflow for preparing patches involves working off a
> topic branch and generating patches against 'master' to send off to the
> maintainer. However, a plain
>
> $ git format-patch -o outgoing
>
> is a no-op on a topic branch, and the user has to remember to specify
> 'master' explicitly everytime. This problem is not unique to
> format-patch; even a
>
> $ git rebase -i
>
> is a no-op because the branch to rebase against isn't specified.
>
> To tackle this problem, introduce branch.*.forkedFrom which can specify
> the parent branch of a topic branch. Future patches will build
> functionality around this new configuration variable.
>
> Cc: Jeff King
> Cc: Junio C Hamano
> Signed-off-by: Ramkumar Ramachandra
> ---
> Since -M, -C, -D are left in the argc, checking argc < 2 isn't
> sufficient.
>
> I wanted to get an early reaction before wiring up checkout and
> rebase.
>
> But I wanted to discuss the overall idea of the patch.
> builtin/log.c | 21 +
> t/t4014-format-patch.sh | 20
> 2 files changed, 41 insertions(+)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index b97373d..525e696 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -674,6 +674,7 @@ static int thread;
> static int do_signoff;
> static const char *signature = git_version_string;
> static int config_cover_letter;
> +static const char *config_base_branch;
>
> enum {
> COVER_UNSET,
> @@ -750,6 +751,22 @@ static int git_format_config(const char *var, const char
> *value, void *cb)
> config_cover_letter = git_config_bool(var, value) ? COVER_ON
> : COVER_OFF;
> return 0;
> }
> + if (starts_with(var, "branch.")) {
> + const char *name = var + 7;
> + const char *subkey = strrchr(name, '.');
> + struct strbuf buf = STRBUF_INIT;
> +
> + if (!subkey)
> + return 0;
> + strbuf_add(&buf, name, subkey - name);
> + if (branch_get(buf.buf) != branch_get(NULL))
> + return 0;
> + strbuf_release(&buf);
> + if (!strcmp(subkey, ".forkedfrom")) {
> + if (git_config_string(&config_base_branch, var,
> value))
> + return -1;
> + }
> + }
>
> return git_log_config(var, value, cb);
> }
> @@ -1324,6 +1341,10 @@ int cmd_format_patch(int argc, const char **argv,
> const char *prefix)
> die (_("--subject-prefix and -k are mutually exclusive."));
> rev.preserve_subject = keep_subject;
>
> + if (argc < 2 && config_base_branch) {
> + argv[1] = config_base_branch;
> + argc++;
> + }
> argc = setup_revisions(argc, argv, &rev, &s_r_opt);
> if (argc > 1)
> die (_("unrecognized argument: %s"), argv[1]);
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 73194b2..2ea94af 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1370,4 +1370,24 @@ test_expect_success 'cover letter auto user override' '
> test_line_count = 2 list
> '
>
> +test_expect_success 'branch.*.forkedFrom matches' '
> + mkdir -p tmp &&
> + test_when_finished "rm -rf tmp;
> + git config --unset branch.rebuild-1.forkedFrom" &&
> +
> + git config branch.rebuild-1.forkedFrom master &&
> + git format-patch -o tmp >list &&
> + test_line_count = 2 list
> +'
> +
> +test_expect_success 'branch.*.forkedFrom does not match' '
> + mkdir -p tmp &&
> + test_when_finished "rm -rf tmp;
> + git config --unset branch.foo.forkedFrom" &&
> +
> + git config branch.foo.forkedFrom master &&
> + git format-patch -o tmp >list &&
> + test_line_count = 0 list
> +'
> +
> test_done
> --
> 1.8.5.2.234.gba2dde8.dirty
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/PATCH] format-patch: introduce branch.*.forkedFrom
A very common workflow for preparing patches involves working off a
topic branch and generating patches against 'master' to send off to the
maintainer. However, a plain
$ git format-patch -o outgoing
is a no-op on a topic branch, and the user has to remember to specify
'master' explicitly everytime. This problem is not unique to
format-patch; even a
$ git rebase -i
is a no-op because the branch to rebase against isn't specified.
To tackle this problem, introduce branch.*.forkedFrom which can specify
the parent branch of a topic branch. Future patches will build
functionality around this new configuration variable.
Cc: Jeff King
Cc: Junio C Hamano
Signed-off-by: Ramkumar Ramachandra
---
Since -M, -C, -D are left in the argc, checking argc < 2 isn't
sufficient.
I wanted to get an early reaction before wiring up checkout and
rebase.
But I wanted to discuss the overall idea of the patch.
builtin/log.c | 21 +
t/t4014-format-patch.sh | 20
2 files changed, 41 insertions(+)
diff --git a/builtin/log.c b/builtin/log.c
index b97373d..525e696 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -674,6 +674,7 @@ static int thread;
static int do_signoff;
static const char *signature = git_version_string;
static int config_cover_letter;
+static const char *config_base_branch;
enum {
COVER_UNSET,
@@ -750,6 +751,22 @@ static int git_format_config(const char *var, const char
*value, void *cb)
config_cover_letter = git_config_bool(var, value) ? COVER_ON :
COVER_OFF;
return 0;
}
+ if (starts_with(var, "branch.")) {
+ const char *name = var + 7;
+ const char *subkey = strrchr(name, '.');
+ struct strbuf buf = STRBUF_INIT;
+
+ if (!subkey)
+ return 0;
+ strbuf_add(&buf, name, subkey - name);
+ if (branch_get(buf.buf) != branch_get(NULL))
+ return 0;
+ strbuf_release(&buf);
+ if (!strcmp(subkey, ".forkedfrom")) {
+ if (git_config_string(&config_base_branch, var, value))
+ return -1;
+ }
+ }
return git_log_config(var, value, cb);
}
@@ -1324,6 +1341,10 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
die (_("--subject-prefix and -k are mutually exclusive."));
rev.preserve_subject = keep_subject;
+ if (argc < 2 && config_base_branch) {
+ argv[1] = config_base_branch;
+ argc++;
+ }
argc = setup_revisions(argc, argv, &rev, &s_r_opt);
if (argc > 1)
die (_("unrecognized argument: %s"), argv[1]);
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 73194b2..2ea94af 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1370,4 +1370,24 @@ test_expect_success 'cover letter auto user override' '
test_line_count = 2 list
'
+test_expect_success 'branch.*.forkedFrom matches' '
+ mkdir -p tmp &&
+ test_when_finished "rm -rf tmp;
+ git config --unset branch.rebuild-1.forkedFrom" &&
+
+ git config branch.rebuild-1.forkedFrom master &&
+ git format-patch -o tmp >list &&
+ test_line_count = 2 list
+'
+
+test_expect_success 'branch.*.forkedFrom does not match' '
+ mkdir -p tmp &&
+ test_when_finished "rm -rf tmp;
+ git config --unset branch.foo.forkedFrom" &&
+
+ git config branch.foo.forkedFrom master &&
+ git format-patch -o tmp >list &&
+ test_line_count = 0 list
+'
+
test_done
--
1.8.5.2.234.gba2dde8.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] drop unnecessary copying in credential_ask_one
On Tue, Jan 07, 2014 at 11:44:00AM -0800, Junio C Hamano wrote: > > test-terminal only handles stdout and stderr streams as fake terminals. > > We could pretty easily add stdin for input, as it uses fork() to work > > asynchronously. But the credential code does not actually read from > > stdin. It opens and reads from /dev/tty explicitly. So I think we'd have > > to actually fake setting up a controlling terminal. And that means magic > > with setsid() and ioctl(TIOCSCTTY), which in turn sounds like a > > portability headache. > > I wonder if "expect" has already solved that for us. I would not be surprised if it did. Though it introduces its own portability issues, since we cannot depend on having it. But it is probably enough to just test_lazy_prereq EXPECT 'expect --version' or something. I dunno. I have never used expect, do not have it installed, and am not excited about introducing a new tool dependency. But if you want to explore it, be my guest. -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] sha1_name: don't resolve refs when core.warnambiguousrefs is false
On Tue, Jan 07, 2014 at 11:38:15AM -0800, Junio C Hamano wrote: > >> > Alternatively, I guess "cat-file > >> > --batch" could just turn off warn_ambiguous_refs itself. > >> > >> Sounds like a sensible way to go, perhaps on top of this change? > > > > The downside is that we would not warn about ambiguous refs anymore, > > even if the user was expecting it to. I don't know if that matters much. > > That is true already with or without Brodie's change, isn't it? > With warn_on_object_refname_ambiguity, "cat-file --batch" makes us > ignore core.warnambigousrefs setting. If we redo 25fba78d > (cat-file: disable object/refname ambiguity check for batch mode, > 2013-07-12) to unconditionally disable warn_ambiguous_refs in > "cat-file --batch" and get rid of warn_on_object_refname_ambiguity, > the end result would be the same, no? No, I don't think the end effect is the same (or maybe we are not talking about the same thing. :) ). There are two ambiguity situations: 1. Ambiguous non-fully-qualified refs (e.g., same tag and head name). 2. 40-hex sha1 object names which might also be unqualified ref names. Prior to 25ffba78d, cat-file checked both (like all the rest of git). But checking (2) is very expensive, since otherwise a 40-hex sha1 does not need to do a ref lookup at all, and something like "rev-list --objects | cat-file --batch-check" processes a large number of these. Detecting (1) is not nearly as expensive. You must already be doing a ref lookup to trigger it (so the relative cost is much closer), and your query size is bounded by the number of refs, not the number of objects. Commit 25ffba78d traded off some safety for a lot of performance by disabling (2), but left (1) in place because the tradeoff is different. The two options I was musing over earlier today were (all on top of Brodie's patch): a. Revert 25ffba78d. With Brodie's patch, core.warnAmbiguousRefs disables _both_ warnings. So we default to safe-but-slow, but people who care about performance can turn off ambiguity warnings. The downside is that you have to know to turn it off manually (and it's a global config flag, so you end up turning it off _everywhere_, not just in big queries where it matters). b. Revert 25ffba78d, but then on top of it just turn off warn_ambiguous_refs unconditionally in "cat-file --batch-check". The downside is that we drop the safety from (1). The upside is that the code is a little simpler, as we drop the extra flag. And obviously: c. Just leave it at Brodie's patch with nothing else on top. My thinking in favor of (b) was basically "does anybody actually care about ambiguous refs in this situation anyway?". If they do, then I think (c) is my preferred choice. > > I kind of feel in the --batch situation that it is somewhat useless (I > > wonder if "rev-list --stdin" should turn it off, too). > > I think doing the same as "cat-file --batch" in "rev-list --stdin" > makes sense. Both interfaces are designed to grok extended SHA-1s, > and full 40-hex object names could be ambiguous and we are missing > the warning for them. I'm not sure I understand what you are saying. We _do_ have the warning for "rev-list --stdin" currently. We do _not_ have the warning for "cat-file --batch", since my 25ffba78d. I was wondering if rev-list should go the same way as 25ffba78d, for efficiency reasons (e.g., think piping to "rev-list --no-walk --stdin"). > Or are you wondering if we should revert 25fba78d, apply Brodie's > change to skip the ref resolution whose result is never used, and > tell people who want to use "cat-file --batch" (or "rev-list > --stdin") to disable the ambiguity warning themselves? See above. :) -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 v2] submodule: Respect requested branch on all clones
2014/1/7 Francesco Pretto : > 2014/1/7 Junio C Hamano : >> It is not immediately obvious to me why anybody who specifies the >> submodule.*.branch variable to say "I want _that_ branch" not to >> want to be on that branch but in a detached state, so from that >> perspective, submodule.*.attach feels superfluous. >> > > Junio, for what it concerns me I fully support *this* patch as, Where "this" patch is Trevor's one, don't get me wrong... :) -- 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 v2] submodule: Respect requested branch on all clones
On Tue, Jan 07, 2014 at 11:21:28AM -0800, Junio C Hamano wrote: > "W. Trevor King" writes: > > On Tue, Jan 07, 2014 at 10:15:25AM -0800, Junio C Hamano wrote: > >> Having writing all the above and then looking at the patch again, it > >> is not immediately obvious to me where you use "rebase" when doing > >> the initial checkout, though. > > > > It's used to shift the local branch reference from from the > > (arbitrary) cloned remote branch tip to the explicit submodule $sha1. > > The objective is not what I was questioning. In the patch I see > > git checkout -f -q -B "$branch" "origin/$branch" > > used in the module_clone (which I think makes sense), and then > cmd_update uses "git reset --hard -q" to make sure that the working > tree matches the commit $sha1 obtained from the superproject's > gitlink (which may make $branch diverged from origin/$branch, but > still I think it makes sense). > > But there is no 'rebase' I can see in the codepath, which was what I > was puzzled about. Ah, thanks. s/rebase/reset/ in the commit message ;). Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [RFC v2] submodule: Respect requested branch on all clones
2014/1/7 W. Trevor King : >> Junio, for what it concerns me I fully support this patch as, IMO, it >> makes cleaner the role of the property "submodule..branch". > > No. Trevor, maybe it was not clear. But I wanted to say: " I fully support *Trevor's* patch..." :) -- 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 v2] submodule: Respect requested branch on all clones
On Tue, Jan 07, 2014 at 08:19:49PM +0100, Francesco Pretto wrote: > 2014/1/7 Junio C Hamano : > > It is not immediately obvious to me why anybody who specifies the > > submodule.*.branch variable to say "I want _that_ branch" not to > > want to be on that branch but in a detached state, so from that > > perspective, submodule.*.attach feels superfluous. > > Junio, for what it concerns me I fully support this patch as, IMO, it > makes cleaner the role of the property "submodule..branch". No, submodule..branch is the name of the remote-tracking branch for 'update --remote'. In this patch, I'm using it as a hint for the preferred local branch name [1], which I now think was a bad idea. After [2], I think that we should just define the preferred local branch name explicitly (submodule..local-branch?). Cheers, Trevor [1]: http://article.gmane.org/gmane.comp.version-control.git/239980 [2]: http://article.gmane.org/gmane.comp.version-control.git/240097 -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/2] Introduce git submodule attached update
2014/1/7 Junio C Hamano : > Francesco Pretto writes: >> The developer does it voluntarily, at his responsibility, because he >> may decide to partecipate more actively to the development of the >> submodule and still want to use a simple "git submodule update" to >> updates his submodules, overriding its configuration as it can be done >> for other properties like, for example, "branch". > > It is still unclear to me why we need attached/detached mode for > that. The developer may want to do an exploratory development, > whose result is unknown to deserve to be committed on the specified > branch at the beginning, and choose to build on a detached HEAD, > which is a perfectly normal thing to do. But the standard way to do > so, whether the developer is working in the top-level superproject > or in a submodule, would be to just do: > > cd $there && git checkout HEAD^0 > > or use whatever commit the state to be detached is at instead of > "HEAD" in the above example, no? > Because of the overlapping change with the the other patch proposed by Trevor, and to not generate confusion, I will stop for now pursuing for an "attach|detach" command/switch specific for submodules, waiting for Trevors's patch possible acceptance. After that I will see it still makes sense or not. -- 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] drop unnecessary copying in credential_ask_one
Jeff King writes: > On Thu, Jan 02, 2014 at 11:08:51AM -0800, Junio C Hamano wrote: > >> Jeff King writes: >> >> > ... But the test suite, of course, always uses askpass because it >> > cannot rely on accessing a terminal (we'd have to do some magic with >> > lib-terminal, I think). >> > >> > So it doesn't detect the problem in your patch, but I wonder if it is >> > worth applying the patch below anyway, as it makes the test suite >> > slightly more robust. >> >> Sounds like a good first step in the right direction. Thanks. > > I took a brief look at adding "real" terminal tests for the credential > code using our test-terminal/lib-terminal.sh setup. Unfortunately, it > falls short of what we need. > > test-terminal only handles stdout and stderr streams as fake terminals. > We could pretty easily add stdin for input, as it uses fork() to work > asynchronously. But the credential code does not actually read from > stdin. It opens and reads from /dev/tty explicitly. So I think we'd have > to actually fake setting up a controlling terminal. And that means magic > with setsid() and ioctl(TIOCSCTTY), which in turn sounds like a > portability headache. I wonder if "expect" has already solved that for us. > So it's definitely possible under Linux, and probably under most Unixes. > But I'm not sure it's worth the effort, given that review already caught > the potential bug here. > > Another option would be to instrument git_terminal_prompt with a > mock-terminal interface (say, reading from a file specified in an > environment variable). But I really hate polluting the code with test > cruft, and it would not actually be testing an interesting segment of > the code, anyway. Agreed. -- 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] sha1_name: don't resolve refs when core.warnambiguousrefs is false
Jeff King writes: > On Tue, Jan 07, 2014 at 09:51:07AM -0800, Junio C Hamano wrote: > >> Jeff King writes: >> >> > Alternatively, I guess "cat-file >> > --batch" could just turn off warn_ambiguous_refs itself. >> >> Sounds like a sensible way to go, perhaps on top of this change? > > The downside is that we would not warn about ambiguous refs anymore, > even if the user was expecting it to. I don't know if that matters much. That is true already with or without Brodie's change, isn't it? With warn_on_object_refname_ambiguity, "cat-file --batch" makes us ignore core.warnambigousrefs setting. If we redo 25fba78d (cat-file: disable object/refname ambiguity check for batch mode, 2013-07-12) to unconditionally disable warn_ambiguous_refs in "cat-file --batch" and get rid of warn_on_object_refname_ambiguity, the end result would be the same, no? > I kind of feel in the --batch situation that it is somewhat useless (I > wonder if "rev-list --stdin" should turn it off, too). I think doing the same as "cat-file --batch" in "rev-list --stdin" makes sense. Both interfaces are designed to grok extended SHA-1s, and full 40-hex object names could be ambiguous and we are missing the warning for them. Or are you wondering if we should revert 25fba78d, apply Brodie's change to skip the ref resolution whose result is never used, and tell people who want to use "cat-file --batch" (or "rev-list --stdin") to disable the ambiguity warning themselves? -- 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] Introduce git submodule attached update
On Mon, Jan 06, 2014 at 08:21:24PM +0100, David Engster wrote: > +---+ > | master | <-- > +---++---+| Merges to/from master > | CEDET | | done only by CEDET developers > +---+ | > +---+| > | stable | <-- < > +---+ | > | > | > | Any Emacs developer > | can push and commit > | submodule > +++--+ | > | Emacs | -- | lisp/cedet submodule | <- > +++--+ This looks reasonable, and except for the detached-HEAD after the initial update-clone, I think Git already supports everything you need. If you set submodule.cedet.update to 'rebase' (or 'merge') you can easily integrate your local master changes with cedet/master (e.g. if a CEDET dev updates cedet/master before the Emacs dev has a chance to push their fix). With the non-checkout update mode, you'll also stay on your checked-out master branch during 'submodule update' calls. > AFAICS the main problem with this approach is that one always has to > think of committing the new SHA1 of the submodule. > … > However, as Heiko notes, the history must be preserved to be able to > go back to earlier revisions, so there must be some kind of commit > for the submodule when 'stable' changes; maybe that could be > automated somehow? If an Emacs dev in the submodule makes the CEDET change, you could use a post-commit hook (in the CEDET submodule) to also commit the change to the Emacs superproject). However, commiting only the submodule bump may not be what you want. Maybe there are other superproject changes that should be committed alongside the submodule bump. Maybe there is stuff in the superprojects's staging area that should *not* be committed alongside the submodule bump. This ambiguity makes it tricky for Git to automatically do “the right thing”. If cedet/master is updated independently by the CEDET devs, there's no way for the local Emacs repo to know about the change, so it's impossible to automatically update Emacs (without polling for CEDET updates or some other transgression ;). Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [PATCH 2/2] Introduce git submodule attached update
2014/1/7 Junio C Hamano : > Francesco Pretto writes: > >> My bottom line: >> - For what I understand, detached HEAD it's a way to say "hey, you >> have to stay on this commit. Also don't even think you can push to the >> upstream branch". This sometimes can't be spurious, as in the use case >> I wrote above: access control on the remote repositories should be >> enough. I think maintainers should have the option to make developers >> to clone a repository starting with an attached HEAD on the branch >> suggested in submodule.$name.branch; >> - "git submodule update" is missing a property to do automatically >> "--remote". I think in the use case I wrote it's really handy to have >> a "git submodule update" to act like this. > > The short version I read in the message is that your workflow, in > which partipants want to work on a branch, gets frustrating with the > current system only because the default update/initial cloning > detaches HEAD and will stay in that state until the user gets out of > the detached state manually. Once that initial detachment is fixed, > there is no more major issue, as update will stay on that branch. > > Am I reading you correctly? > Yep, you got it correctly. -- 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] sha1_name: don't resolve refs when core.warnambiguousrefs is false
This change ensures get_sha1_basic() doesn't try to resolve full hashes
as refs when ambiguous ref warnings are disabled.
This provides a substantial performance improvement when passing many
hashes to a command (like "git rev-list --stdin") when
core.warnambiguousrefs is false. The check incurs 6 stat()s for every
hash supplied, which can be costly over NFS.
Signed-off-by: Brodie Rao
---
sha1_name.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index e9c2999..10bd007 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -451,9 +451,9 @@ static int get_sha1_basic(const char *str, int len,
unsigned char *sha1)
int at, reflog_len, nth_prior = 0;
if (len == 40 && !get_sha1_hex(str, sha1)) {
- if (warn_on_object_refname_ambiguity) {
+ if (warn_ambiguous_refs && warn_on_object_refname_ambiguity) {
refs_found = dwim_ref(str, len, tmp_sha1, &real_ref);
- if (refs_found > 0 && warn_ambiguous_refs) {
+ if (refs_found > 0) {
warning(warn_msg, len, str);
if (advice_object_name_warning)
fprintf(stderr, "%s\n",
_(object_name_msg));
--
1.8.5.2
--
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 v2] submodule: Respect requested branch on all clones
"W. Trevor King" writes: > On Tue, Jan 07, 2014 at 10:15:25AM -0800, Junio C Hamano wrote: >> submodule: respect requested branch on all clones >> >> The previous code only checked out the requested branch in cmd_add >> but not in cmd_update; this left the user on a detached HEAD after >> an update initially cloned, and subsequent updates using rebase or >> merge mode will kept the HEAD detached, unless the user moved to the >> desired branch himself. >> >> Move the branch-checkout logic into module_clone, where it can be >> shared by cmd_add and cmd_update. Also update the initial checkout >> command to use 'rebase' to preserve branches setup during >> module_clone. This way, unless the user explicitly asks to work on >> a detached HEAD, subsequent updates all happen on the specified >> branch, which matches the end-user expectation much better. > > This looks reasonable to me, but there are still changes I'd like to > make for a v3 (e.g. using submodule..update to trigger local > branch checkout). However, I'm currently leaning towards a new 'git > submodule checkout' command with explicit preferred local submodule > branches (see [1]). Maybe this should all wait until Jens rolls out > his update implementation [2]? Sounds good. I'll backburner this one, then. >> Having writing all the above and then looking at the patch again, it >> is not immediately obvious to me where you use "rebase" when doing >> the initial checkout, though. > > It's used to shift the local branch reference from from the > (arbitrary) cloned remote branch tip to the explicit submodule $sha1. The objective is not what I was questioning. In the patch I see git checkout -f -q -B "$branch" "origin/$branch" used in the module_clone (which I think makes sense), and then cmd_update uses "git reset --hard -q" to make sure that the working tree matches the commit $sha1 obtained from the superproject's gitlink (which may make $branch diverged from origin/$branch, but still I think it makes sense). But there is no 'rebase' I can see in the codepath, which was what I was puzzled about. -- 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 v2] submodule: Respect requested branch on all clones
2014/1/7 Junio C Hamano : >> Unless you decide to go with the proposed approach of Trevor, where >> "submodule..branch" set means attached (if it's not changed: >> this thread is quite hard to follow...). To this end, Junio could sync >> with more "long-timers" (Heiko?) submodule users/devs to understand if >> this breaks too much or not. > > It is not immediately obvious to me why anybody who specifies the > submodule.*.branch variable to say "I want _that_ branch" not to > want to be on that branch but in a detached state, so from that > perspective, submodule.*.attach feels superfluous. > Junio, for what it concerns me I fully support this patch as, IMO, it makes cleaner the role of the property "submodule..branch". Because with my original proposal I decided to go non-breaking Heiko and Jens could also take position on this because this patch will represent a small behavior break. Also, and important feature should be added together with this patch: a way to go "--remote" by default on an attached HEAD. This can be done at least in two ways: - explicit, non breaking way: add a "submodule..remote" property. When set to "true" it implies "--remote" when doing "git submodule update", both on attached and detached HEAD; - implicit, breaking way: assume "--remote" when doing "git submodule update" on an attached HEAD. I am quite sure this will break a couple of submodule tests (I already tried it), probably for marginal reasons. I think this is needed because it makes little sense to having an attached HEAD and "git submodule update" does nothing. Thank you, Francesco -- 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] Introduce git submodule attached update
Francesco Pretto writes: > My bottom line: > - For what I understand, detached HEAD it's a way to say "hey, you > have to stay on this commit. Also don't even think you can push to the > upstream branch". This sometimes can't be spurious, as in the use case > I wrote above: access control on the remote repositories should be > enough. I think maintainers should have the option to make developers > to clone a repository starting with an attached HEAD on the branch > suggested in submodule.$name.branch; > - "git submodule update" is missing a property to do automatically > "--remote". I think in the use case I wrote it's really handy to have > a "git submodule update" to act like this. The short version I read in the message is that your workflow, in which partipants want to work on a branch, gets frustrating with the current system only because the default update/initial cloning detaches HEAD and will stay in that state until the user gets out of the detached state manually. Once that initial detachment is fixed, there is no more major issue, as update will stay on that branch. Am I reading you correctly? -- 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] Introduce git submodule attached update
Francesco Pretto writes: >>> > - In which situations does the developer or maintainer switch between >>> >your attached/detached mode? >>> >>> The developer/maintainer does so optionally and voluntarily and it >>> effects only its private working tree. >> >> This does not answer my question. I would like to find out the reason >> why one would do the switch. > > The developer does it voluntarily, at his responsibility, because he > may decide to partecipate more actively to the development of the > submodule and still want to use a simple "git submodule update" to > updates his submodules, overriding its configuration as it can be done > for other properties like, for example, "branch". It is still unclear to me why we need attached/detached mode for that. The developer may want to do an exploratory development, whose result is unknown to deserve to be committed on the specified branch at the beginning, and choose to build on a detached HEAD, which is a perfectly normal thing to do. But the standard way to do so, whether the developer is working in the top-level superproject or in a submodule, would be to just do: cd $there && git checkout HEAD^0 or use whatever commit the state to be detached is at instead of "HEAD" in the above example, no? -- 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 v2] submodule: Respect requested branch on all clones
On Tue, Jan 07, 2014 at 10:15:25AM -0800, Junio C Hamano wrote: > submodule: respect requested branch on all clones > > The previous code only checked out the requested branch in cmd_add > but not in cmd_update; this left the user on a detached HEAD after > an update initially cloned, and subsequent updates using rebase or > merge mode will kept the HEAD detached, unless the user moved to the > desired branch himself. > > Move the branch-checkout logic into module_clone, where it can be > shared by cmd_add and cmd_update. Also update the initial checkout > command to use 'rebase' to preserve branches setup during > module_clone. This way, unless the user explicitly asks to work on > a detached HEAD, subsequent updates all happen on the specified > branch, which matches the end-user expectation much better. This looks reasonable to me, but there are still changes I'd like to make for a v3 (e.g. using submodule..update to trigger local branch checkout). However, I'm currently leaning towards a new 'git submodule checkout' command with explicit preferred local submodule branches (see [1]). Maybe this should all wait until Jens rolls out his update implementation [2]? > Having writing all the above and then looking at the patch again, it > is not immediately obvious to me where you use "rebase" when doing > the initial checkout, though. It's used to shift the local branch reference from from the (arbitrary) cloned remote branch tip to the explicit submodule $sha1. Otherwise the default method for that operation is a HEAD-detaching 'checkout'. I tried to explain it here [3]. > "W. Trevor King" writes: > > The current Documentation/git-submodule.txt has: > > > > update:: > > Update the registered submodules, i.e. clone missing submodules > > and checkout the commit specified in the index of the containing > > repository. This will make the submodules HEAD be detached unless > > `--rebase` or `--merge` is specified or the key > > `submodule.$name.update` is set to `rebase`, `merge` or `none`. > > Side note but doesn't Francesco's "'checkout' is a valid update mode" > need to update this part of the documentation as well? That would be nice. I don't think his patch changes the docs, and I don't know if mentioning the --checkout option belongs in that patch as well, or in a separate fixup ;). Cheers, Trevor [1]: http://article.gmane.org/gmane.comp.version-control.git/240097 [2]: http://article.gmane.org/gmane.comp.version-control.git/240117 [3]: http://article.gmane.org/gmane.comp.version-control.git/239953 -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: [PATCH v3] stash: handle specifying stashes with spaces
Øystein Walle writes:
> When trying to pop/apply a stash specified with an argument containing
> spaces git-stash will throw an error:
>
> $ git stash pop 'stash@{two hours ago}'
> Too many revisions specified: stash@{two hours ago}
>
> This happens because word splitting is used to count non-option
> arguments. Make use of rev-parse's --sq option to quote the arguments
> for us to ensure a correct count. Add quotes where necessary.
>
> Also add a test that verifies correct behaviour.
>
> Helped-by: Thomas Rast
> Signed-off-by: Øystein Walle
> ---
> v3 uses the same eval/--sq technique as v2, suggested by Thomas Rast.
> This is basically a resend except that I added a missing '&&' in the
> test that Eric Sunshine noticed when reading v1.
The change looks good.
An unrelated tangent, but here is a tip-of-the-day for the
approximate parser. You could have just said
git stash pop stash@{2.hours}
and it would have been interpreted just the same ;-)
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index debda7a..7eb011c 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -673,4 +673,15 @@ test_expect_success 'store updates stash ref and reflog'
> '
> grep quux bazzy
> '
>
> +test_expect_success 'handle stash specification with spaces' '
> + git stash clear &&
> + echo pig > file &&
Style: no SP between redirection operator and its target, i.e.
echo pig >file &&
> + git stash &&
> + test_tick &&
> + echo cow > file &&
> + git stash &&
> + git stash apply "stash@{Thu Apr 7 15:17:13 2005 -0700}" &&
This is brittle. If new tests are added before this, the test_tick
will give you different timestamp and this test will start failing.
Perhaps grab the timestamp out of the stash that was created, e.g.
...
test_tick &&
git stash &&
stamp=$(git log -g --format="%cd" -1 refs/stash) &&
test_tick &&
echo cow >file &&
git stash &&
git stash apply "stash@{$stamp}" &&
or something?
> + grep pig file
> +'
> +
> test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2] submodule: Respect requested branch on all clones
Francesco Pretto writes: > 2014/1/7 Francesco Pretto : >> To not break the existing behavior what it's really needed here, IMO, >> is a "submodule..attached" property that says two things: >> - at the first clone on "git submodule update" stay attached to >> "submodule..branch"; >> - implies "--remote", as it's the only thing that makes sense when the >> submodules are attached. >> > > Unless you decide to go with the proposed approach of Trevor, where > "submodule..branch" set means attached (if it's not changed: > this thread is quite hard to follow...). To this end, Junio could sync > with more "long-timers" (Heiko?) submodule users/devs to understand if > this breaks too much or not. It is not immediately obvious to me why anybody who specifies the submodule.*.branch variable to say "I want _that_ branch" not to want to be on that branch but in a detached state, so from that perspective, submodule.*.attach feels superfluous. But I'd mostly defer the design discussion to Heiko (and Jens), WTK and you. -- 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 v2] submodule: Respect requested branch on all clones
"W. Trevor King" writes:
> From: "W. Trevor King"
>
> The previous code only checked out the requested branch in cmd_add.
> This commit moves the branch-checkout logic into module_clone, where
> it can be shared by cmd_add and cmd_update. I also update the initial
> checkout command to use 'rebase' to preserve branches setup during
> module_clone.
I want to see the log message explain the motivation behind it
(i.e. instead of stopping after saying "We used to do X, now we do
Y", but also explain why we consider that Y is better than X). Here
is my attempt.
submodule: respect requested branch on all clones
The previous code only checked out the requested branch in cmd_add
but not in cmd_update; this left the user on a detached HEAD after
an update initially cloned, and subsequent updates using rebase or
merge mode will kept the HEAD detached, unless the user moved to the
desired branch himself.
Move the branch-checkout logic into module_clone, where it can be
shared by cmd_add and cmd_update. Also update the initial checkout
command to use 'rebase' to preserve branches setup during
module_clone. This way, unless the user explicitly asks to work on
a detached HEAD, subsequent updates all happen on the specified
branch, which matches the end-user expectation much better.
Signed-off-by: W. Trevor King
Signed-off-by: Junio C Hamano
Please correct me if I misunderstood the intention.
Having writing all the above and then looking at the patch again, it
is not immediately obvious to me where you use "rebase" when doing
the initial checkout, though.
> The current Documentation/git-submodule.txt has:
>
> update::
> Update the registered submodules, i.e. clone missing submodules
> and checkout the commit specified in the index of the containing
> repository. This will make the submodules HEAD be detached unless
> `--rebase` or `--merge` is specified or the key
> `submodule.$name.update` is set to `rebase`, `merge` or `none`.
Side note but doesn't Francesco's "'checkout' is a valid update mode"
need to update this part of the documentation as well?
> git-submodule.sh | 34 --
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 2979197..167d4fa 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -253,6 +253,7 @@ module_clone()
> url=$3
> reference="$4"
> depth="$5"
> + branch="$6"
> quiet=
> if test -n "$GIT_QUIET"
> then
> @@ -306,7 +307,15 @@ module_clone()
> echo "gitdir: $rel/$a" >"$sm_path/.git"
>
> rel=$(echo $a | sed -e 's|[^/][^/]*|..|g')
> - (clear_local_git_env; cd "$sm_path" && GIT_WORK_TREE=. git config
> core.worktree "$rel/$b")
> + (
> + clear_local_git_env
> + cd "$sm_path" &&
> + GIT_WORK_TREE=. git config core.worktree "$rel/$b" &&
> + case "$branch" in
> + '') git checkout -f -q ;;
> + ?*) git checkout -f -q -B "$branch" "origin/$branch" ;;
> + esac
> + ) || die "$(eval_gettext "Unable to setup cloned submodule
> '\$sm_path'")"
> }
>
> isnumber()
> @@ -469,16 +478,7 @@ Use -f if you really want to add it." >&2
> echo "$(eval_gettext "Reactivating local git
> directory for submodule '\$sm_name'.")"
> fi
> fi
> - module_clone "$sm_path" "$sm_name" "$realrepo" "$reference"
> "$depth" || exit
> - (
> - clear_local_git_env
> - cd "$sm_path" &&
> - # ash fails to wordsplit ${branch:+-b "$branch"...}
> - case "$branch" in
> - '') git checkout -f -q ;;
> - ?*) git checkout -f -q -B "$branch" "origin/$branch" ;;
> - esac
> - ) || die "$(eval_gettext "Unable to checkout submodule
> '\$sm_path'")"
> + module_clone "$sm_path" "$sm_name" "$realrepo" "$reference"
> "$depth" "$branch" || exit
> fi
> git config submodule."$sm_name".url "$realrepo"
>
> @@ -787,7 +787,8 @@ cmd_update()
> fi
> name=$(module_name "$sm_path") || exit
> url=$(git config submodule."$name".url)
> - branch=$(get_submodule_config "$name" branch master)
> + config_branch=$(get_submodule_config "$name" branch)
> + branch="${config_branch:-master}"
> if ! test -z "$update"
> then
> update_module=$update
> @@ -815,7 +816,7 @@ Maybe you want to use 'update --init'?")"
>
> if ! test -d "$sm_path"/.git -o -f "$sm_path"/.git
> then
> - module_clone "$sm_path" "$name" "$url" "$reference"
> "$depth" || exit
> + module_clone "$sm_path" "$na
Re: [PATCH] pager: set LV=-c alongside LESS=FRSX
Junio C Hamano writes: > - Scripted Porcelains get LESS=-FRSX while C Porcelains get >LESS=FRSX as the default (the leading dash being the >difference), which looks somewhat inconsistent. Not a new >problem, though. Even the less manpage is inconsistent about whether $LESS should start with a dash (there are examples with and without it). Implementation-wise, less uses the same function to process an option argument (including the leading dash) and the value of $LESS, so the form with the leading dash is probably preferred. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." -- 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] mv: better document side effects when moving a submodule
Am 06.01.2014 23:40, schrieb Junio C Hamano: > Jens Lehmann writes: >> Does this new paragraph make it clearer? > > Don't we have bugs section that we can use to list the known > limitations like this? Right, will change accordingly in v2. >> Documentation/git-mv.txt | 10 ++ >> t/t7001-mv.sh| 21 + > > It also may make sense to express the test as "this is what we would > like to see happen eventually" in the form of test_expect_failure; > it is not a big deal though. We'll get the "what we would like to see happen eventually" test for free from the recursive submodule update framework I'm currently implementing, so I propose we don't implement this exepected failure in this patch. -- 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] Fix safe_create_leading_directories() for Windows
Hi, On Tue, 7 Jan 2014, Erik Faye-Lund wrote: > On Thu, Jan 2, 2014 at 9:46 PM, Johannes Schindelin > wrote: > > > Well, you and I both know how easy GitHub's pull request made things > > for us as well as for contributors. I really cannot thank Erik enough > > for bullying me into using and accepting them. > > Huh? I don't think you refer to me, because I really dislike them (and I > always have IIRC). Ah yes, I misremembered. You were actually opposed to using them and I thought we should be pragmatic to encourage contributions. In any case, I do think that the contributions we got via pull requests were in general contributions we would not otherwise have gotten. Sorry for my mistake! Dscho -- 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] sha1_name: don't resolve refs when core.warnambiguousrefs is false
On Tue, Jan 07, 2014 at 09:51:07AM -0800, Junio C Hamano wrote: > Jeff King writes: > > > Alternatively, I guess "cat-file > > --batch" could just turn off warn_ambiguous_refs itself. > > Sounds like a sensible way to go, perhaps on top of this change? The downside is that we would not warn about ambiguous refs anymore, even if the user was expecting it to. I don't know if that matters much. I kind of feel in the --batch situation that it is somewhat useless (I wonder if "rev-list --stdin" should turn it off, too). -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] sha1_name: don't resolve refs when core.warnambiguousrefs is false
Jeff King writes: > Alternatively, I guess "cat-file > --batch" could just turn off warn_ambiguous_refs itself. Sounds like a sensible way to go, perhaps on top of this change? -- 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] drop unnecessary copying in credential_ask_one
On Thu, Jan 02, 2014 at 11:08:51AM -0800, Junio C Hamano wrote: > Jeff King writes: > > > ... But the test suite, of course, always uses askpass because it > > cannot rely on accessing a terminal (we'd have to do some magic with > > lib-terminal, I think). > > > > So it doesn't detect the problem in your patch, but I wonder if it is > > worth applying the patch below anyway, as it makes the test suite > > slightly more robust. > > Sounds like a good first step in the right direction. Thanks. I took a brief look at adding "real" terminal tests for the credential code using our test-terminal/lib-terminal.sh setup. Unfortunately, it falls short of what we need. test-terminal only handles stdout and stderr streams as fake terminals. We could pretty easily add stdin for input, as it uses fork() to work asynchronously. But the credential code does not actually read from stdin. It opens and reads from /dev/tty explicitly. So I think we'd have to actually fake setting up a controlling terminal. And that means magic with setsid() and ioctl(TIOCSCTTY), which in turn sounds like a portability headache. So it's definitely possible under Linux, and probably under most Unixes. But I'm not sure it's worth the effort, given that review already caught the potential bug here. Another option would be to instrument git_terminal_prompt with a mock-terminal interface (say, reading from a file specified in an environment variable). But I really hate polluting the code with test cruft, and it would not actually be testing an interesting segment of the code, anyway. -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: What's cooking in git.git (Jan 2014, #01; Mon, 6)
Am 06.01.2014 23:36, schrieb Junio C Hamano: > * jl/submodule-recursive-checkout (2013-12-26) 5 commits > - Teach checkout to recursively checkout submodules > - submodule: teach unpack_trees() to update submodules > - submodule: teach unpack_trees() to repopulate submodules > - submodule: teach unpack_trees() to remove submodule contents > - submodule: prepare for recursive checkout of submodules > > What is the doneness of this one??? It's still work in progress. Currently I'm working on a test framework so we can reuse recursive submodule checkout tests instead of rewriting them for every command that learns the --recurse-submodule option. Will reroll this series as soon as I have something presentable. -- 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-submodule.sh: Support 'checkout' as a valid update command
2014/1/7 Junio C Hamano : > It is not about preference but what we want to convey to the > readers. When you start the sentence with "Oh, it already works > correctly", the readers need to see this sentence finished: "It > already works, it is handled correctly, but we change the code > nevertheless because ...?". > > Here is my attempt to fill that "because ..." part: > > Subject: git-submodule.sh: 'checkout' is a valid update mode > > 'checkout' is documented as one of the valid values for > 'submodule..update' variable, and in a repository with > the variable set to 'checkout', "git submodule update" > command do update using the 'checkout' mode. > > However, it has been an accident that the implementation > works this way; any unknown value would trigger the same > codepath and update using the 'checkout' mode. > > Tighten the codepath and explicitly list 'checkout' as one > of the known update modes, and error out when an unknown > update mode is used. > > Also, teach the codepath that initializes the configuration > variable from in-tree .gitmodules that 'checkout' is one of > the valid values---the code since ac1fbbda (submodule: do > not copy unknown update mode from .gitmodules, 2013-12-02) > used to treat the value 'checkout' as unknown and mapped it > to 'none', which made little sense. > I wouldn't be able to explain the change better than your description. Also, I was under the improper assumption that the change was obvious. Thank you very much for the amended patch description. Cheers, Francesco -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/17] safe_create_leading_directories(): add explicit "slash" pointer
Michael Haggerty writes: > I agree that it would be reasonable to use is_dir_sep() in the > implementation of this function, at least unless/until somebody does the > work to figure out whether callers should really only be passing it > forward-slash-normalized paths. > > Please be careful, though, because I don't think this function is > capable of handling arbitrary Windows paths, like for example > //host/path format, either before or after my change. > > Let me know if you would like me to merge or rebase the is_dir_sep() > changes into this patch series. I'd want SSchuberth and windows folks to be at least aware of this series and preferrably see that they offer inputs to this series, making their is_dir_sep() change just one step in this series. That way I have one less series to worry about ;-) Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [RFC] Making use of bitmaps for thin objects
On Mon, Jan 06, 2014 at 10:14:30PM +, Ben Maurer wrote: > It looks like for my repo the size win wasn't as big (~10%). Is it > possible that with the kernel test you got extremely lucky and there > was some huge binary blob that thin packing turned into a tiny delta? I don't think so. When I look at the reused-delta numbers, I see that we reused ~3000 extra deltas (and the "compressing" progress meter drops by the same amount. If I do a full clone (or just index-pack the result), it claims ~3000 thin objects completed with local objects (versus 0 in the normal case). So I think we really are getting a lot of little savings adding up, which makes sense. If there were thousands of changed files, a non-thin pack has to have at least _one_ full version of each file. With thin packs, we might literally have only deltas. It was a 7-week period, which might make more difference. I'm going to run some experiments with different time periods to see if that changes anything. It might also be the repo contents. I'm going to try my experiments on a few different repositories. It may be that either the kernel or your repo is unusual in some way. Or maybe I was just lucky. :) > When you get a chance, it'd be handy if you could push an updated > version of your change out to your public github repo. I'd like to see > if folks here are interested in testing this more, and it'd be good to > make sure we're testing the diff that is targeted for upstream. I've pushed it to: git://github.com/peff/git.git jk/bitmap-reuse-delta I'll continue to rebase it forward as time goes on (until a cleaned-up version gets merged upstream), but the tip of that branch should always be in a working state. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 13/17] remove_dir_recurse(): handle disappearing files and directories
Michael Haggerty writes: > I'm not sure I understand your point. Please note that the > REMOVE_DIR_KEEP_TOPLEVEL bit is cleared from flags before this function > recurses. So in recursive invocations, keep_toplevel will always be > false, and the rmdir(path->buf) codepath will be permitted. Does that > answer your question? Yes; thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sha1_name: don't resolve refs when core.warnambiguousrefs is false
Brodie Rao writes:
> This change ensures get_sha1_basic() doesn't try to resolve full hashes
> as refs when ambiguous ref warnings are disabled.
>
> This provides a substantial performance improvement when passing many
> hashes to a command (like "git rev-list --stdin") when
> core.warnambiguousrefs is false. The check incurs 6 stat()s for every
> hash supplied, which can be costly over NFS.
> ---
Needs sign-off. The patch looks good.
Thanks.
> sha1_name.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index e9c2999..10bd007 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -451,9 +451,9 @@ static int get_sha1_basic(const char *str, int len,
> unsigned char *sha1)
> int at, reflog_len, nth_prior = 0;
>
> if (len == 40 && !get_sha1_hex(str, sha1)) {
> - if (warn_on_object_refname_ambiguity) {
> + if (warn_ambiguous_refs && warn_on_object_refname_ambiguity) {
> refs_found = dwim_ref(str, len, tmp_sha1, &real_ref);
> - if (refs_found > 0 && warn_ambiguous_refs) {
> + if (refs_found > 0) {
> warning(warn_msg, len, str);
> if (advice_object_name_warning)
> fprintf(stderr, "%s\n",
> _(object_name_msg));
--
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] sha1_name: don't resolve refs when core.warnambiguousrefs is false
On Mon, Jan 06, 2014 at 07:35:04PM -0800, Brodie Rao wrote: > On Mon, Jan 6, 2014 at 7:32 PM, Brodie Rao wrote: > > This change ensures get_sha1_basic() doesn't try to resolve full hashes > > as refs when ambiguous ref warnings are disabled. > > > > This provides a substantial performance improvement when passing many > > hashes to a command (like "git rev-list --stdin") when > > core.warnambiguousrefs is false. The check incurs 6 stat()s for every > > hash supplied, which can be costly over NFS. > > Forgot to add: > > Signed-off-by: Brodie Rao Looks good to me. I wonder if I should have simply gone this route instead of adding warn_on_object_refname_ambiguity, and then people who want "cat-file --batch" to be fast could just turn off core.warnAmbiguousRefs. I wanted it to happen automatically, though. Alternatively, I guess "cat-file --batch" could just turn off warn_ambiguous_refs itself. -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] pager: set LV=-c alongside LESS=FRSX
Jonathan Nieder writes:
> On systems with lv configured as the preferred pager (i.e.,
> DEFAULT_PAGER=lv at build time, or PAGER=lv exported in the
> environment) git commands that use color show control codes instead of
> color in the pager:
>
> $ git diff
> ^[[1mdiff --git a/.mailfilter b/.mailfilter^[[m
> ^[[1mindex aa4f0b2..17e113e 100644^[[m
> ^[[1m--- a/.mailfilter^[[m
> ^[[1m+++ b/.mailfilter^[[m
> ^[[36m@@ -1,11 +1,58 @@^[[m
>
> "less" avoids this problem because git uses the LESS environment
> variable to pass the -R option ('output ANSI color escapes in raw
> form') by default. Use the LV environment variable to pass 'lv' the
> -c option ('allow ANSI escape sequences for text decoration / color')
> to fix it for lv, too.
>
> Noticed when the default value for color.ui flipped to 'auto' in
> v1.8.4-rc0~36^2~1 (2013-06-10).
>
> Reported-by: Olaf Meeuwissen
> Signed-off-by: Jonathan Nieder
> ---
> Olaf Meeuwissen wrote[1]:
>
>> Yes, it's called LV and documented in the lv(1) manual page. Simply
>> search for 'env' ;-)
>
> Ah, thanks. How about this patch?
>
> [1] http://bugs.debian.org/730527
Looks good; though I have to wonder two (and a half) things:
- Scripted Porcelains get LESS=-FRSX while C Porcelains get
LESS=FRSX as the default (the leading dash being the
difference), which looks somewhat inconsistent. Not a new
problem, though.
- Can we generalize this a bit so that a builder can pass a list
of var=val pairs and demote the existing LESS=FRSX to just a
canned setting of such a mechanism?
- Can such a code be reused to make this into a runtime setting,
even? Would it be worth the complexity?
Thanks.
> Documentation/config.txt | 4
> git-sh-setup.sh | 3 ++-
> pager.c | 11 +--
> perl/Git/SVN/Log.pm | 1 +
> t/t7006-pager.sh | 12
> 5 files changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index a405806..ed59853 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -567,6 +567,10 @@ be passed to the shell by Git, which will translate the
> final
> command to `LESS=FRSX less -+S`. The environment tells the command
> to set the `S` option to chop long lines but the command line
> resets it to the default to fold long lines.
> ++
> +Likewise, when the `LV` environment variable is unset, Git sets it
> +to `-c`. You can override this setting by exporting `LV` with
> +another value or setting `core.pager` to `lv +c`.
>
> core.whitespace::
> A comma separated list of common whitespace problems to
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 190a539..fffa3c7 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -159,7 +159,8 @@ git_pager() {
> GIT_PAGER=cat
> fi
> : ${LESS=-FRSX}
> - export LESS
> + : ${LV=-c}
> + export LESS LV
>
> eval "$GIT_PAGER" '"$@"'
> }
> diff --git a/pager.c b/pager.c
> index 345b0bc..0cc75a8 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -80,8 +80,15 @@ void setup_pager(void)
> pager_process.use_shell = 1;
> pager_process.argv = pager_argv;
> pager_process.in = -1;
> - if (!getenv("LESS")) {
> - static const char *env[] = { "LESS=FRSX", NULL };
> + if (!getenv("LESS") || !getenv("LV")) {
> + static const char *env[3];
> + int i = 0;
> +
> + if (!getenv("LESS"))
> + env[i++] = "LESS=FRSX";
> + if (!getenv("LV"))
> + env[i++] = "LV=-c";
> + env[i] = NULL;
> pager_process.env = env;
> }
> if (start_command(&pager_process))
> diff --git a/perl/Git/SVN/Log.pm b/perl/Git/SVN/Log.pm
> index 3f8350a..34f2869 100644
> --- a/perl/Git/SVN/Log.pm
> +++ b/perl/Git/SVN/Log.pm
> @@ -117,6 +117,7 @@ sub run_pager {
> }
> open STDIN, '<&', $rfd or fatal "Can't redirect stdin: $!";
> $ENV{LESS} ||= 'FRSX';
> + $ENV{LV} ||= '-c';
> exec $pager or fatal "Can't run pager: $! ($pager)";
> }
>
> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index ff25908..7fe3367 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -37,6 +37,18 @@ test_expect_failure TTY 'pager runs from subdir' '
> test_cmp expected actual
> '
>
> +test_expect_success TTY 'LESS and LV envvars are set for pagination' '
> + (
> + sane_unset LESS LV &&
> + PAGER="env >pager-env.out" &&
> + export PAGER &&
> +
> + test_terminal git log
> + ) &&
> + grep ^LESS= pager-env.out &&
> + grep ^LV= pager-env.out
> +'
> +
> test_expect_success TTY 'some commands do not use a pager' '
> rm -f paginated.out &&
> test_terminal git rev-list HEAD &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.ker
Re: [PATCH 2/2] format-patch: introduce format.defaultTo
John Szakmeister wrote:
> I guess it's not a good idea to
> set 'push.default' to 'upstream' in this triangle workflow then,
> otherwise the branch name being pushed to will be 'branch.*.merge'.
> Is that correct? I just want to make sure I understand what's
> happening here.
push.default = upstream does not support triangular workflows. See
builtin/push.c:setup_push_upstream().
> Given this new found knowledge, I'm not sure it makes sense for `git
> status` to show me the status against the upstream vs. the publish
> location. The latter makes a little more sense to me, though I see
> the usefulness of either one.
Currently, status information is only against @{u}; we haven't
invented a @{publish} yet.
--
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-submodule.sh: Support 'checkout' as a valid update command
Francesco Pretto writes: > Like you said, "it already refers to checkout and handles it > correctly". I think the use of the simple present tense here is > correct: it's a fact. Feel free to advice another wording if you > prefer. It is not about preference but what we want to convey to the readers. When you start the sentence with "Oh, it already works correctly", the readers need to see this sentence finished: "It already works, it is handled correctly, but we change the code nevertheless because ...?". Here is my attempt to fill that "because ..." part: Subject: git-submodule.sh: 'checkout' is a valid update mode 'checkout' is documented as one of the valid values for 'submodule..update' variable, and in a repository with the variable set to 'checkout', "git submodule update" command do update using the 'checkout' mode. However, it has been an accident that the implementation works this way; any unknown value would trigger the same codepath and update using the 'checkout' mode. Tighten the codepath and explicitly list 'checkout' as one of the known update modes, and error out when an unknown update mode is used. Also, teach the codepath that initializes the configuration variable from in-tree .gitmodules that 'checkout' is one of the valid values---the code since ac1fbbda (submodule: do not copy unknown update mode from .gitmodules, 2013-12-02) used to treat the value 'checkout' as unknown and mapped it to 'none', which made little sense. -- 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] Fix safe_create_leading_directories() for Windows
On Thu, Jan 2, 2014 at 9:46 PM, Johannes Schindelin wrote: > Hi Sebastian, > > On Thu, 2 Jan 2014, Sebastian Schuberth wrote: > >> On 02.01.2014 18:33, Johannes Schindelin wrote: >> >> > -- snip -- >> > On Linux, we can get away with assuming that the directory separator is a >> > forward slash, but that is wrong in general. For that purpose, the >> > is_dir_sep() function was introduced a long time ago. By using it in >> > safe_create_leading_directories(), we proof said function for use on >> > platforms where the directory separator is different from Linux'. >> > -- snap -- >> >> While I'd be fine with this, I do not think we really need it. > > I also would have been fine with your commit message. But I knew Junio > wouldn't be. > >> As you say, is_dir_sep() has been introduced a long time ago, so people >> should be aware of it, and it should also be immediately clear from the >> diff why using it is better than hard-coding '/'. >> >> That said, I see any further explanations on top of the commit message >> title is an added bonus, and as "just" a bonus a link to a pull request >> should be fine. You don't need to understand or appreciate the concept >> of pull requests in order to follow the link and read the text in there. > > Well, you and I both know how easy GitHub's pull request made things for > us as well as for contributors. I really cannot thank Erik enough for > bullying me into using and accepting them. Huh? I don't think you refer to me, because I really dislike them (and I always have IIRC). -- 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/2] completion: complete format.coverLetter
Ramkumar Ramachandra wrote: > contrib/completion/git-completion.bash | 1 + > 1 file changed, 1 insertion(+) Junio: Please push this part via 'maint' independently. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 17/17] rename_tmp_log(): on SCLD_VANISHED, retry
On 01/06/2014 07:21 PM, Junio C Hamano wrote: > Michael Haggerty writes: > >> If safe_create_leading_directories() fails because a file along the >> path unexpectedly vanished, try again from the beginning. Try at most >> 3 times. > > As the previous step bumped it from 3 to 4 without explanation, the > above no longer reflects reality ;-) Good catch. The increment 3 -> 4 was because the first call to rename() is optimistic, and can fail once even if there is no race. I will change the commit message of 16/17 to explain this point, and of 17/17 to match reality. > The series mostly looked sane from a cursory read. > > Will re-queue. Thanks. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 10/17] lock_ref_sha1_basic(): on SCLD_VANISHED, retry
On 01/06/2014 06:54 PM, Junio C Hamano wrote:
> Michael Haggerty writes:
>
>> If safe_create_leading_directories() fails because a file along the
>> path unexpectedly vanished, try again (up to 3 times).
>>
>> This can occur if another process is deleting directories at the same
>> time as we are trying to make them. For example, "git pack-refs
>> --all" tries to delete the loose refs and any empty directories that
>> are left behind. If a pack-refs process is running, then it might
>> delete a directory that we need to put a new loose reference in.
>>
>> If safe_create_leading_directories() thinks this might have happened,
>> then take its advice and try again (maximum three attempts).
>>
>> Signed-off-by: Michael Haggerty
>> ---
>> refs.c | 11 ++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 3926136..6eb8a02 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2039,6 +2039,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
>> *refname,
>> int type, lflags;
>> int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
>> int missing = 0;
>> +int attempts = 3;
>>
>> lock = xcalloc(1, sizeof(struct ref_lock));
>> lock->lock_fd = -1;
>> @@ -2093,7 +2094,15 @@ static struct ref_lock *lock_ref_sha1_basic(const
>> char *refname,
>> if ((flags & REF_NODEREF) && (type & REF_ISSYMREF))
>> lock->force_write = 1;
>>
>> -if (safe_create_leading_directories(ref_file)) {
>> + retry:
>> +switch (safe_create_leading_directories(ref_file)) {
>> +case SCLD_OK:
>> +break; /* success */
>> +case SCLD_VANISHED:
>> +if (--attempts > 0)
>> +goto retry;
>> +/* fall through */
>
> Hmph.
>
> Having no backoff/sleep at all might be OK here as long as the other
> side that removes does not retry (and I do not think the other side
> would, even though I haven't read through the series to the end yet
> ;-)).
remove_dir_recurse() only tries deleting directories once (I haven't
changed that). And from a broader perspective, it would be pretty silly
for any tidy-up-directories function to try deleting things more than
once. So I don't think it is a problem. But even in the worst case,
this function only tries three times before giving up, so it shouldn't
be a disaster.
> This may be just a style thing, but I find that the variable name
> "attempts" that starts out as 3 quite misleading, as its value is
> not "the number of attempts made" but "the remaining number of
> attempts allowed." Starting it from 0 and then
>
> if (attempts++ < MAX_ATTEMPTS)
> goto retry;
>
> would be one way to clarify it. Renaming it to remaining_attempts
> would be another.
I just renamed the variable to attempts_remaining. (I thought I was
following your suggestion, but now I see that I put the words in the
opposite order; oh well, I think it's fine either way.)
Thanks for your review! I will wait a day or so for any additional
comments, and then send a v3.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 13/17] remove_dir_recurse(): handle disappearing files and directories
On 01/06/2014 07:18 PM, Junio C Hamano wrote:
> Michael Haggerty writes:
>
>> If a file or directory that we are trying to remove disappears (e.g.,
>> because another process has pruned it), do not consider it an error.
>>
>> Signed-off-by: Michael Haggerty
>> ---
>> dir.c | 22 --
>> 1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/dir.c b/dir.c
>> index 11e1520..716b613 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1476,7 +1476,9 @@ static int remove_dir_recurse(struct strbuf *path, int
>> flag, int *kept_up)
>> flag &= ~REMOVE_DIR_KEEP_TOPLEVEL;
>> dir = opendir(path->buf);
>> if (!dir) {
>> -if (errno == EACCES && !keep_toplevel)
>> +if (errno == ENOENT)
>> +return keep_toplevel ? -1 : 0;
>
> If we were told that "foo/bar must go, but do not bother removing
> foo/", is it an error if foo itself did not exist? I think this
> step does not change the behaviour from the original (we used to say
> "oh, we were told to keep_toplevel, and the top-level cannot be read
> for whatever reason, so it is an error"), but because this series is
> giving us a finer grained error diagnosis, we may want to think
> about it further, perhaps as a follow-up.
Indeed, this is a design choice that I should have explained. I
implemented it this way to keep the behavior the same as the old code in
this situation, and because I thought that if the caller explicitly asks
for the toplevel path to be kept, and that path doesn't even exist at
the entrance to the function, then something is screwy and it is better
to return an error than to keep going.
It would even be possible to argue that if keep_toplevel is set but path
is missing, then this function should call
safe_create_leading_directories(path).
Changing this behavior would require an audit to see which behavior
would be most useful to the callers. I think that is out of the scope
of this patch series.
> I also wonder why the keep-toplevel logic is in this "recurse" part
> of the callchain. If everything in "foo/bar/" can be removed but
> "foo/bar/" is unreadable, it is OK, when opendir("foo/bar") failed
> with EACCESS, to attempt to rmdir("foo/bar") whether we were told
> not to attempt removing "foo/", no?
>
>> +else if (errno == EACCES && !keep_toplevel)
>
> That is, I am wondering why this part just checks !keep_toplevel,
> not
>
> (!keep_toplevel || has_dir_separator(path->buf))
>
> or something.
I'm not sure I understand your point. Please note that the
REMOVE_DIR_KEEP_TOPLEVEL bit is cleared from flags before this function
recurses. So in recursive invocations, keep_toplevel will always be
false, and the rmdir(path->buf) codepath will be permitted. Does that
answer your question?
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/17] safe_create_leading_directories(): add explicit "slash" pointer
On 01/06/2014 07:32 PM, Junio C Hamano wrote: > Michael Haggerty writes: > >> Keep track of the position of the slash character independently of >> "pos", thereby making the purpose of each variable clearer and >> working towards other upcoming changes. >> >> Signed-off-by: Michael Haggerty >> --- > > This step has an interaction with $gmane/239878 where Windows folks > want it to pay attention to is_dir_sep()---over there, a backslash > could separate directory path components. > > AFAIK, the function was meant to be used only on paths we internally > generate, and the paths we internally generate all are slash > separated, so it could be argued that feeding a path, whose path > components are separated by backslashes, that we obtained from the > end user without converting it to the internal form in some > codepaths (e.g. "$there" in "git clone $url $there") are bugs we > acquired over time that need to be fixed, but it is easy enough to > use is_dir_sep() here to work it around, and doing so will > not negatively affect > > 1. UNIX-only projects by forbidding use of a byte with backslash in > it as a path component character (yes, I am imagining using > Shift-JIS that can use a backslash as the second byte of > two-byte character in the pathname on UNIX); and > > 2. UNIX-and-Windows mixed projects, as you cannot sanely use such a > pathname with backslash as part of a path component if its tree > needs to be checked out on Windows. I agree that it would be reasonable to use is_dir_sep() in the implementation of this function, at least unless/until somebody does the work to figure out whether callers should really only be passing it forward-slash-normalized paths. Please be careful, though, because I don't think this function is capable of handling arbitrary Windows paths, like for example //host/path format, either before or after my change. Let me know if you would like me to merge or rebase the is_dir_sep() changes into this patch series. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] stash: handle specifying stashes with spaces
When trying to pop/apply a stash specified with an argument containing
spaces git-stash will throw an error:
$ git stash pop 'stash@{two hours ago}'
Too many revisions specified: stash@{two hours ago}
This happens because word splitting is used to count non-option
arguments. Make use of rev-parse's --sq option to quote the arguments
for us to ensure a correct count. Add quotes where necessary.
Also add a test that verifies correct behaviour.
Helped-by: Thomas Rast
Signed-off-by: Øystein Walle
---
v3 uses the same eval/--sq technique as v2, suggested by Thomas Rast.
This is basically a resend except that I added a missing '&&' in the
test that Eric Sunshine noticed when reading v1.
git-stash.sh | 14 +++---
t/t3903-stash.sh | 11 +++
2 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/git-stash.sh b/git-stash.sh
index 1e541a2..f0a94ab 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -358,7 +358,7 @@ parse_flags_and_rev()
i_tree=
u_tree=
- REV=$(git rev-parse --no-flags --symbolic "$@") || exit 1
+ REV=$(git rev-parse --no-flags --symbolic --sq "$@") || exit 1
FLAGS=
for opt
@@ -376,7 +376,7 @@ parse_flags_and_rev()
esac
done
- set -- $REV
+ eval set -- $REV
case $# in
0)
@@ -391,13 +391,13 @@ parse_flags_and_rev()
;;
esac
- REV=$(git rev-parse --quiet --symbolic --verify $1 2>/dev/null) || {
+ REV=$(git rev-parse --quiet --symbolic --verify "$1" 2>/dev/null) || {
reference="$1"
die "$(eval_gettext "\$reference is not valid reference")"
}
- i_commit=$(git rev-parse --quiet --verify $REV^2 2>/dev/null) &&
- set -- $(git rev-parse $REV $REV^1 $REV: $REV^1: $REV^2: 2>/dev/null) &&
+ i_commit=$(git rev-parse --quiet --verify "$REV^2" 2>/dev/null) &&
+ set -- $(git rev-parse "$REV" "$REV^1" "$REV:" "$REV^1:" "$REV^2:"
2>/dev/null) &&
s=$1 &&
w_commit=$1 &&
b_commit=$2 &&
@@ -408,8 +408,8 @@ parse_flags_and_rev()
test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${REV%@*}")"
&&
IS_STASH_REF=t
- u_commit=$(git rev-parse --quiet --verify $REV^3 2>/dev/null) &&
- u_tree=$(git rev-parse $REV^3: 2>/dev/null)
+ u_commit=$(git rev-parse --quiet --verify "$REV^3" 2>/dev/null) &&
+ u_tree=$(git rev-parse "$REV^3:" 2>/dev/null)
}
is_stash_like()
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index debda7a..7eb011c 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -673,4 +673,15 @@ test_expect_success 'store updates stash ref and reflog' '
grep quux bazzy
'
+test_expect_success 'handle stash specification with spaces' '
+ git stash clear &&
+ echo pig > file &&
+ git stash &&
+ test_tick &&
+ echo cow > file &&
+ git stash &&
+ git stash apply "stash@{Thu Apr 7 15:17:13 2005 -0700}" &&
+ grep pig file
+'
+
test_done
--
1.8.5
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
