Re: [PATCH v4 01/10] ref-filter: add option to align atoms to the left
On Mon, Jul 27, 2015 at 5:18 PM, Duy Nguyen wrote:
> On Mon, Jul 27, 2015 at 2:39 PM, Jacob Keller wrote:
>> On Sun, Jul 26, 2015 at 5:39 PM, Duy Nguyen wrote:
>>> On Sun, Jul 26, 2015 at 11:08 AM, Eric Sunshine
>>> wrote:
>>>> You can generate an interdiff with "git diff branchname-v4
>>>> branchname-v5", for instance.
>>>
>>> Off topic. But what stops me from doing this often is it creates a big
>>> mess in "git tag -l". Do we have an option to hide away some
>>> "insignificant:" tags? reflog might be an option if we have something
>>> like foo@{/v2} to quickly retrieve the reflog entry whose message
>>> contains "v2".
> ...
>
> But maybe we're abusing reflog..
Actually a good place for this stuff is "git branch
--edit-description". A lot of manual steps to save old refs, do
inter-diff.. but it's probably good enough.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Fwd: Bug#793884: git: allows nonsensical command 'git checkout -b HEAD'
This seems like a good thing to fix (i.e. make sure XX is not ambiguous before creating it with "git checkout -b XX") -- Forwarded message -- From: Andreas Beckmann Date: Tue, Jul 28, 2015 at 9:18 PM Subject: Bug#793884: git: allows nonsensical command 'git checkout -b HEAD' To: Debian Bug Tracking System Package: git Version: 1:2.1.4-2.1 Severity: normal Tags: upstream $ git branch HEAD fatal: it does not make sense to create 'HEAD' manually # OK, special casing prevents this $ git checkout -b HEAD Switched to a new branch 'HEAD' # but not this :-P $ git checkout master Switched to branch 'master' $ git checkout HEAD warning: refname 'HEAD' is ambiguous. Switched to branch 'HEAD' # oops ;-) $ git checkout master Switched to branch 'master' $ git branch -d HEAD Deleted branch HEAD (was 6e54945). # OK, we can easily cleanup this mess again The same works in 1:2.4.6-1 in sid. If there is some special casing for HEAD in git branch, the same should probably be done for git checkout -b, too. Andreas -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Log messages beginning # and git rebase -i
On Wed, Jul 29, 2015 at 12:48 AM, Matthieu Moy wrote: >> If the user wants whatever she types in the resulting commit >> literally, there is the "--cleanup=" option, no? > > $ GIT_EDITOR=touch git commit --cleanup=verbatim > [detached HEAD 1b136a7] # Please enter the commit message for your changes. > Lines starting # with '#' will be kept; you may remove them yourself if you > want > to. # An empty message aborts the commit. # HEAD detached from 5e70007 # > Changes to be committed: # modified: foo.txt # # Changes not staged for > commit > : # modified: foo.txt # # Untracked files: # last-synchro.txt # > 1 file changed, 1 insertion(+), 1 deletion(-) > > You really don't want that in day-to-day use. How about --cleanup=scissors? The chance that you have the same cut line in your commit message is really low, compared to having comment characters. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Log messages beginning # and git rebase -i
On Wed, Jul 29, 2015 at 7:17 PM, Matthieu Moy wrote: > Duy Nguyen writes: > >> On Wed, Jul 29, 2015 at 12:48 AM, Matthieu Moy >> wrote: >>>> If the user wants whatever she types in the resulting commit >>>> literally, there is the "--cleanup=" option, no? >>> >>> $ GIT_EDITOR=touch git commit --cleanup=verbatim >>> [detached HEAD 1b136a7] # Please enter the commit message for your changes. >>> Lines starting # with '#' will be kept; you may remove them yourself if you >>> want >>> to. # An empty message aborts the commit. # HEAD detached from 5e70007 # >>> Changes to be committed: # modified: foo.txt # # Changes not staged >>> for commit >>> : # modified: foo.txt # # Untracked files: # last-synchro.txt # >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> You really don't want that in day-to-day use. >> >> How about --cleanup=scissors? > > I can read this in two different ways: > > 1) Keeping git as-is and suggest users to use --cleanup=scissors > >This has the same problem as --cleanup=verbatim: it doesn't work as-is >since Git doesn't insert the scissors. You can hack around it by >adding them by yourself when you need it, but it's really not >convenient. You have to anticipate that you're going to require a # >and call commit with --cleanup=scissors, add the scissors. And repeat >it if you need to "commit --amend". > > 2) Modify Git to add scissors by default, and use --cleanup=scissors by >default. > >This is actually more or less what SVN does: it inserts a line >"--This line, and those below, will be ignored--", and the equivalent >of what Git adds as comments in the template is inserted below this >line. > > I don't think option 1) is good. The fact that we have the --cleanup= > option shouldn't serve as an excuse to do nothing. I'd be fine with > option 2), but I find it much more intrusive than to allow a simple > backslash-escaping as I suggest. auto backslashing could cause some annoyance. Emacs supports rearranging a paragraph to fit in a fixed text column. This generated backslash may be moved around, no longer at the beginning of the line, and it will remain in the commit message. I don't know how popular this feature is outside emacs. Having said that, even scissors has its own (and probably bigger) problem: when you commit after conflict resolution, git inserts a "Conflicts:" paragraph, prepended by core.commentChar. With default settings, it serves as a reminder, but will be automatically stripped. With scissors, it stays by default because it's placed before the scissor line. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Fwd: New Defects reported by Coverity Scan for git
Jeff, I suppose you are the admin of git on scan.coverity, or knows
him/her, perhaps we can add a model for xmalloc to suppress these
"null pointer deferences" reports? We are sure xmalloc() never returns
NULL. Qemu did it [1] and it looks simple.. I think something like
this would do
void *xmalloc(size_t size)
{
void *mem = malloc(size);
if (!mem) __coverity_panic__();
return mem;
}
[1]
http://git.qemu.org/?p=qemu.git;a=blob;f=scripts/coverity-model.c;h=4c99a85cfc292caa9edd9d041e2683ee53490a8d;hb=e40cdb0e6efb795e4d19368987d53e3e4ae19cf7#l104
-- Forwarded message --
From:
Date: Fri, Jul 31, 2015 at 5:54 PM
Subject: New Defects reported by Coverity Scan for git
To: pclo...@gmail.com
___
*** CID 1313836: Null pointer dereferences (FORWARD_NULL)
/rerere.c: 150 in find_rerere_dir()
144 return NULL; /* BUG */
145 pos = sha1_pos(sha1, rerere_dir, rerere_dir_nr,
rerere_dir_sha1);
146 if (pos < 0) {
147 rr_dir = xmalloc(sizeof(*rr_dir));
148 hashcpy(rr_dir->sha1, sha1);
149 rr_dir->status_nr = rr_dir->status_alloc = 0;
>>> CID 1313836: Null pointer dereferences (FORWARD_NULL)
>>> Assigning: "rr_dir->status" = "NULL".
150 rr_dir->status = NULL;
151 pos = -1 - pos;
152
153 /* Make sure the array is big enough ... */
154 ALLOC_GROW(rerere_dir, rerere_dir_nr + 1,
rerere_dir_alloc);
155 /* ... and add it in. */
** CID 1313835: Null pointer dereferences (FORWARD_NULL)
/builtin/fetch.c: 795 in prune_refs()
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Confused about sparse vs untracked-cache
On Fri, Jul 31, 2015 at 12:13 PM, David Turner wrote: > I should mention that other than that, skip-worktree + untracked cache > seems to work fine. Please go ahead and make a patch to allow it. I'll spend some more time looking at this code. But I think it'll be fine. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] untracked-cache: support sparse checkout
On Sat, Aug 1, 2015 at 12:35 AM, David Turner wrote:
> Remove a check that would disable the untracked cache for sparse
> checkouts. Add tests that ensure that the untracked cache works with
> sparse checkouts -- specifically considering the case that a file
> foo/bar is checked out, but foo/.gitignore is not.
I have looked some more at the code (sorry for being slow these days,
$DAY_JOB can be exhausting). The reason 27b099a (untracked cache:
don't open non-existent .gitignore - 2015-03-08) avoids skip-worktree
is because when that patch is added, index changes do not affect
untracked cache (yet). So when you delete the on-worktree .gitignore,
untracked cache is invalidated and it falls back to the index version.
exclude_sha1 would reflect the content in the index. If the in-index
.gitignore is deleted, without feedback from the index, untracked
cache remains unchanged (i.e. valid) and assumes that .gitignore is
still there. Which is wrong. That's fixed in e931371 (untracked cache:
invalidate at index addition or removal - 2015-03-08).
With that out of the way,
Acked-by: Duy Nguyen
> Signed-off-by: David Turner
> ---
> dir.c | 17 +-
> t/t7063-status-untracked-cache.sh | 119
> ++
> 2 files changed, 122 insertions(+), 14 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index 8209f8b..e7b89fe 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1078,10 +1078,9 @@ static void prep_exclude(struct dir_struct *dir, const
> char *base, int baselen)
> (!untracked || !untracked->valid ||
> /*
> * .. and .gitignore does not exist before
> - * (i.e. null exclude_sha1 and skip_worktree is
> - * not set). Then we can skip loading .gitignore,
> - * which would result in ENOENT anyway.
> - * skip_worktree is taken care in read_directory()
> + * (i.e. null exclude_sha1). Then we can skip
> + * loading .gitignore, which would result in
> + * ENOENT anyway.
> */
> !is_null_sha1(untracked->exclude_sha1))) {
> /*
> @@ -1880,7 +1879,6 @@ static struct untracked_cache_dir
> *validate_untracked_cache(struct dir_struct *d
> const struct pathspec
> *pathspec)
> {
> struct untracked_cache_dir *root;
> - int i;
>
> if (!dir->untracked || getenv("GIT_DISABLE_UNTRACKED_CACHE"))
> return NULL;
> @@ -1932,15 +1930,6 @@ static struct untracked_cache_dir
> *validate_untracked_cache(struct dir_struct *d
> if (dir->exclude_list_group[EXC_CMDL].nr)
> return NULL;
>
> - /*
> -* An optimization in prep_exclude() does not play well with
> -* CE_SKIP_WORKTREE. It's a rare case anyway, if a single
> -* entry has that bit set, disable the whole untracked cache.
> -*/
> - for (i = 0; i < active_nr; i++)
> - if (ce_skip_worktree(active_cache[i]))
> - return NULL;
> -
> if (!ident_in_untracked(dir->untracked)) {
> warning(_("Untracked cache is disabled on this system."));
> return NULL;
> diff --git a/t/t7063-status-untracked-cache.sh
> b/t/t7063-status-untracked-cache.sh
> index bd4806c..ff23f4e 100755
> --- a/t/t7063-status-untracked-cache.sh
> +++ b/t/t7063-status-untracked-cache.sh
> @@ -354,4 +354,123 @@ EOF
> test_cmp ../expect ../actual
> '
>
> +test_expect_success 'set up for sparse checkout testing' '
> + echo two >done/.gitignore &&
> + echo three >>done/.gitignore &&
> + echo two >done/two &&
> + git add -f done/two done/.gitignore &&
> + git commit -m "first commit"
> +'
> +
> +test_expect_success 'status after commit' '
> + : >../trace &&
> + GIT_TRACE_UNTRACKED_STATS="$TRASH_DIRECTORY/trace" \
> + git status --porcelain >../actual &&
> + cat >../status.expect < +?? .gitignore
> +?? dtwo/
> +EOF
> + test_cmp ../status.expect ../actual &&
> + cat >../trace.expect < +node creation: 0
> +gitignore invalidation: 0
> +directory invalidation: 0
> +opendir: 1
> +EOF
> + test_cmp ../trace.expect ../trace
> +'
> +
> +test_expect_success 'untracked cache correct after commit' '
> + test-dump-untracked-c
Re: [PATCH] untracked-cache: support sparse checkout
On Mon, Aug 3, 2015 at 5:18 PM, Duy Nguyen wrote:
> On Sat, Aug 1, 2015 at 12:35 AM, David Turner
> wrote:
>> Remove a check that would disable the untracked cache for sparse
>> checkouts. Add tests that ensure that the untracked cache works with
>> sparse checkouts -- specifically considering the case that a file
>> foo/bar is checked out, but foo/.gitignore is not.
>
> I have looked some more at the code (sorry for being slow these days,
> $DAY_JOB can be exhausting). The reason 27b099a (untracked cache:
> don't open non-existent .gitignore - 2015-03-08) avoids skip-worktree
> is because when that patch is added, index changes do not affect
> untracked cache (yet). So when you delete the on-worktree .gitignore,
> untracked cache is invalidated and it falls back to the index version.
> exclude_sha1 would reflect the content in the index. If the in-index
> .gitignore is deleted, without feedback from the index, untracked
> cache remains unchanged (i.e. valid) and assumes that .gitignore is
> still there. Which is wrong.
Hmm.. I got it backward: it should be like this: delete .gitignore on
worktree and in index, exclude_sha1 is null. then add .gitignore in
index only. exclude_sha1 remains null because the cache is still valid
even though we should .gitignore from the index.
> That's fixed in e931371 (untracked cache:
> invalidate at index addition or removal - 2015-03-08).
>
> With that out of the way,
>
> Acked-by: Duy Nguyen
>
>> Signed-off-by: David Turner
>> ---
>> dir.c | 17 +-
>> t/t7063-status-untracked-cache.sh | 119
>> ++
>> 2 files changed, 122 insertions(+), 14 deletions(-)
>>
>> diff --git a/dir.c b/dir.c
>> index 8209f8b..e7b89fe 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1078,10 +1078,9 @@ static void prep_exclude(struct dir_struct *dir,
>> const char *base, int baselen)
>> (!untracked || !untracked->valid ||
>> /*
>> * .. and .gitignore does not exist before
>> - * (i.e. null exclude_sha1 and skip_worktree is
>> - * not set). Then we can skip loading .gitignore,
>> - * which would result in ENOENT anyway.
>> - * skip_worktree is taken care in read_directory()
>> + * (i.e. null exclude_sha1). Then we can skip
>> + * loading .gitignore, which would result in
>> + * ENOENT anyway.
>> */
>> !is_null_sha1(untracked->exclude_sha1))) {
>> /*
>> @@ -1880,7 +1879,6 @@ static struct untracked_cache_dir
>> *validate_untracked_cache(struct dir_struct *d
>> const struct pathspec
>> *pathspec)
>> {
>> struct untracked_cache_dir *root;
>> - int i;
>>
>> if (!dir->untracked || getenv("GIT_DISABLE_UNTRACKED_CACHE"))
>> return NULL;
>> @@ -1932,15 +1930,6 @@ static struct untracked_cache_dir
>> *validate_untracked_cache(struct dir_struct *d
>> if (dir->exclude_list_group[EXC_CMDL].nr)
>> return NULL;
>>
>> - /*
>> -* An optimization in prep_exclude() does not play well with
>> -* CE_SKIP_WORKTREE. It's a rare case anyway, if a single
>> -* entry has that bit set, disable the whole untracked cache.
>> -*/
>> - for (i = 0; i < active_nr; i++)
>> - if (ce_skip_worktree(active_cache[i]))
>> - return NULL;
>> -
>> if (!ident_in_untracked(dir->untracked)) {
>> warning(_("Untracked cache is disabled on this system."));
>> return NULL;
>> diff --git a/t/t7063-status-untracked-cache.sh
>> b/t/t7063-status-untracked-cache.sh
>> index bd4806c..ff23f4e 100755
>> --- a/t/t7063-status-untracked-cache.sh
>> +++ b/t/t7063-status-untracked-cache.sh
>> @@ -354,4 +354,123 @@ EOF
>> test_cmp ../expect ../actual
>> '
>>
>> +test_expect_success 'set up for sparse checkout testing' '
>> + echo two >done/.gitignore &&
>> + echo three >>done/.gitignore &&
>> + echo two >done/two &&
>> + git add -f done/two done/.gitignore &&
>> + git commit -m "first commit"
>> +'
>> +
>> +test
Re: [PATCH/RFC 0/2] bisect per-worktree
On Sat, Aug 1, 2015 at 1:51 PM, Michael Haggerty wrote: > For each worktree, we could then create a different view of the > references by splicing parts of the full reference namespace together. > This could even be based on config settings so that we don't have to > hardcode information like "refs/bisect/* is worktree-specific" deep in > the references module. Suppose we could write > > [worktree.refs] > map = refs/worktrees/*: > map = refs/bisect/*:refs/worktrees/[worktree]/refs/bisect/* Perhaps the destination should be worktrees/[worktree]/refs/bisect/*. Does it have to start with "refs/"? > which would mean (a) hide the references under refs/worktrees", and (b) > make it look as if the references under > refs/worktrees/[worktree]/refs/bisect actually appear under refs/bisect > (where "[worktree]" is replaced with the current worktree's name). By > making these settings configurable, we allow other projects to define > their own worktree-specific reference namespaces too. > > The corresponding main repo might hide "refs/worktrees/*" but leave its > refs/bisect namespace exposed in the usual place. > > "git prune" would see the whole namespace as it really is so that it can > compute reachability correctly. For multiple wortrees, first we basically remap paths in .git, relocate some to worktrees/[worktree]/ (already done), then we're going to remap config keys (submodule support, just proof of concept so far), remapping ref paths sounds like the logical next step if some under refs/ needs to be worktree-specific. I wonder if current ref namespace code could be extended to do this? -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 0/2] bisect per-worktree
On Sat, Aug 1, 2015 at 10:59 AM, Michael Haggerty wrote: > Either way, there's also the question of who should know how to find the > worktree-specific references--the program that wants to access them, or > should there be a secret invisible mapping that is done on lookup, and > that knows the full list of references that want to be worktree-specific > (for example, that in a linked worktree, "refs/bisect/*" should be > silently redirected to "refs/worktree//bisect/*")? > > It's all a bit frightening, frankly. I would think all work to make pluggable backends happen should allow us to do this kind of transparent mapping. We can add a new file-based backend with just this redirection, can't we? I haven't read the updated refs.c though, need to do it now.. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] notes: handle multiple worktrees
On Wed, Jul 29, 2015 at 5:50 AM, Johan Herland wrote:
> On Wed, Jul 29, 2015 at 12:12 AM, Junio C Hamano wrote:
>> David Turner writes:
>>> Prevent merges to the same notes branch from different worktrees.
>>> Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using the same
>>> code we use to check that two HEADs in different worktrees don't point
>>> to the same branch. Modify that code, die_if_checked_out, to take a
>>> "head" ref to examine; previously, it just looked at HEAD.
>>>
>>> Reported-by: Junio C Hamano
>>> Signed-off-by: David Turner
>>> ---
>>
>> Thanks for following through. As I didn't report anything, I do not
>> deserve that label, but it's OK ;-)
>>
>> I know that it is a requirement to protect NOTES_MERGE_REF from
>> being used by multiple places for "notes merge" to play well with
>> the multi-worktree world order, but because I do not know if that is
>> sufficient, I'm asking a few people for further review.
>
> As just stated in a related thread, I don't think it makes sense to
> have NOTES_MERGE_REF per worktree, as the notes merge is always
> completely unrelated to the current worktree (or the current branch
> for that matter). AFAICS this patch is all about handling per-worktree
> NOTES_MERGE_REFs, and as such I'm NAK on this patch. Instead, there
> should only be one NOTES_MERGE_REF per _repo_, and although we might
> want to expand to allow multiple concurrent notes merges in the
> future, that is still a per-repo, and not a per-worktree thing, hence
> completely unrelated to David's current effort.
I agree. Luckily sharing NOTES_MERGE_REF is as short as
diff --git a/path.c b/path.c
index 10f4cbf..52d8ee4 100644
--- a/path.c
+++ b/path.c
@@ -94,7 +94,7 @@ static void replace_dir(struct strbuf *buf, int len,
const char *newdir)
static const char *common_list[] = {
"/branches", "/hooks", "/info", "!/logs", "/lost-found",
"/objects", "/refs", "/remotes", "/worktrees", "/rr-cache", "/svn",
- "config", "!gc.pid", "packed-refs", "shallow",
+ "config", "!gc.pid", "packed-refs", "shallow", "NOTES_MERGE_REF",
NULL
};
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/5] refs: add ref_type function
On Fri, Jul 31, 2015 at 1:06 PM, David Turner wrote:
> Add a function ref_type, which categorizes refs as per-worktree,
> pseudoref, or normal ref.
For per-worktree refs, you probably should follow common_list[] in
path.c because that's how file-based ref namespace is splitted between
per-repo and per-worktree, even though just as simple as "everything
outside refs/ is per-worktree" (with an exception of NOTES_MERGE_REF,
which should be on the list as well). At least the two should be
aligned so that the default file-based backend works the same way as
new backends.
Going further, I think you need to pass the "worktree identifier" to
ref backend, at least in ref_transaction_begin_fn. Each backend is
free to store per-worktree refs however it wants. Of course if I ask
for refs/foo of worktree A, you should not return me refs/foo of
worktree B. ref_transaction_begin_fn can return a fault code if it
does not support multiple worktrees, which is fine.
> Later, we will use this in refs.c to treat pseudorefs specially.
> Alternate ref backends may use it to treat both pseudorefs and
> per-worktree refs differently.
I'm not so sure that this can't be hidden behind backends and they can
have total control on falling back to file-based, or store them in
some secondary storage. I haven't re-read your discussion with Junio
yet (only skimmed through long ago) so I may be missing some important
points.
>
> Signed-off-by: David Turner
> ---
> refs.c | 26 ++
> refs.h | 8
> 2 files changed, 34 insertions(+)
>
> diff --git a/refs.c b/refs.c
> index 0b96ece..0f87884 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2848,6 +2848,32 @@ static int delete_ref_loose(struct ref_lock *lock, int
> flag, struct strbuf *err)
> return 0;
> }
>
> +static int is_per_worktree_ref(const char *refname)
> +{
> + return !strcmp(refname, "HEAD");
> +}
> +
> +static int is_pseudoref_syntax(const char *refname)
> +{
> + const char *c;
> +
> + for (c = refname; *c; c++) {
> + if (!isupper(*c) && *c != '-' && *c != '_')
> + return 0;
> + }
> +
> + return 1;
> +}
> +
> +enum ref_type ref_type(const char *refname)
> +{
> + if (is_per_worktree_ref(refname))
> + return REF_TYPE_PER_WORKTREE;
> + if (is_pseudoref_syntax(refname))
> + return REF_TYPE_PSEUDOREF;
> + return REF_TYPE_NORMAL;
> +}
> +
> int delete_ref(const char *refname, const unsigned char *old_sha1,
>unsigned int flags)
> {
> diff --git a/refs.h b/refs.h
> index e4e46c3..dca4fb5 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -445,6 +445,14 @@ extern int parse_hide_refs_config(const char *var, const
> char *value, const char
>
> extern int ref_is_hidden(const char *);
>
> +enum ref_type {
> + REF_TYPE_PER_WORKTREE,
> + REF_TYPE_PSEUDOREF,
> + REF_TYPE_NORMAL,
> +};
> +
> +enum ref_type ref_type(const char *refname);
> +
> enum expire_reflog_flags {
> EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
> EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
> --
> 2.0.4.315.gad8727a-twtrsrc
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 0/2] bisect per-worktree
On Mon, Aug 3, 2015 at 8:02 PM, Duy Nguyen wrote: > On Sat, Aug 1, 2015 at 10:59 AM, Michael Haggerty > wrote: >> Either way, there's also the question of who should know how to find the >> worktree-specific references--the program that wants to access them, or >> should there be a secret invisible mapping that is done on lookup, and After re-reading some code, I think this invisible mapping on lookup is already done for our file-based ref backend. Take resolve_ref_unsafe_1 for example, the call strbuf_git_path() may return ".git/worktrees/foo/HEAD", not ".git/HEAD", given the input ref "HEAD". So ref names are already separated from where they are stored. >> that knows the full list of references that want to be worktree-specific >> (for example, that in a linked worktree, "refs/bisect/*" should be >> silently redirected to "refs/worktree//bisect/*")? My decision to hard code these paths may turn out a bad thing (it's common_list[] in path.c). Perhaps I should let the user extend them, but we will not allow to override a small subset of paths because that may break stuff horribly. >> It's all a bit frightening, frankly. > > I would think all work to make pluggable backends happen should allow > us to do this kind of transparent mapping. We can add a new file-based > backend with just this redirection, can't we? I haven't read the > updated refs.c though, need to do it now.. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 0/2] bisect per-worktree
On Tue, Aug 4, 2015 at 2:49 AM, David Turner wrote: > Simply treating refs/worktree as per-worktree, while the rest of refs/ > is not, would be a few dozen lines of code. The full remapping approach > is likely to be a lot more. I've already got the lmdb backend working > with something like this approach. If we decide on a complicated > approach, I am likely to run out of time to work on pluggable backends. I think you still have another option: decide that lmdb backend does not (yet) support multiple worktrees (which means make "git worktree add" reject when lmdb backend is used). That would simplify some for you and we can continue on at a later time. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
pack negotiation algorithm between 2 share-nothing repos
This is a corner case that has a real use case: git clone linux-2.6.git cd linux-2.6 git remote add history git-history.git git fetch history # graft graft graft Because history.gi and linux-2.6.git have nothing in common, the server side keeps asking for more "have"s and the client keeps sending in "git fetch history". Negotiation phase takes longer than my patience so I abort it, hack git to send no "have"s and retry. I know this is a corner case, but because it has a valid use case, maybe we should do something about it. Immediate reaction is to add an option to send no "have"s. But maybe you guys have better ideas. PS. I know i'm behind my git inbox. Looking into it soon. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] untracked-cache: fix subdirectory handling
First of all, sorry for my long silence. I'm going through my git
inbox now.
> diff --git a/dir.c b/dir.c
> index e7b89fe..314080b 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -631,6 +631,7 @@ static struct untracked_cache_dir
> *lookup_untracked(struct untracked_cache *uc,
> memset(d, 0, sizeof(*d));
> memcpy(d->name, name, len);
> d->name[len] = '\0';
> + d->depth = dir->depth + 1;
>
> ALLOC_GROW(dir->dirs, dir->dirs_nr + 1, dir->dirs_alloc);
> memmove(dir->dirs + first + 1, dir->dirs + first,
> @@ -1324,7 +1325,19 @@ static enum path_treatment treat_directory(struct
> dir_struct *dir,
> if (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))
> return exclude ? path_excluded : path_untracked;
>
> - untracked = lookup_untracked(dir->untracked, untracked, dirname, len);
> + if (untracked) {
> + const char *cur = dirname;
> + int i;
> +
> + for (i = 0; i < untracked->depth; i++) {
> + cur = strchr(cur, '/');
> + assert(cur);
> + cur++;
> + }
> + untracked = lookup_untracked(dir->untracked, untracked,
> + cur,
> + len - (cur - dirname));
> + }
If I read it correctly, the call chain is, treat_path() reconstructs
full path, then it calls treat_one_path -> treat_directory ->
lookup_untracked.
In the same treat_path(), we may also call treat_fast_path ->
read_directory_recursive -> lookup_untracked. In this call chain, we
retain the "baselen" and we can exclude the base path before passing
it to lookup_untracked().
So instead of adding "depth" (and spending some more cycles cutting
path components), perhaps we can do the same for the first call chain,
passing baselen to treat_one_path and treat_directory? Something like
this passes your new tests
-- 8< --
diff --git a/dir.c b/dir.c
index 07a6642..a4c52bf 100644
--- a/dir.c
+++ b/dir.c
@@ -1297,7 +1297,7 @@ static enum exist_status directory_exists_in_index(const
char *dirname, int len)
*/
static enum path_treatment treat_directory(struct dir_struct *dir,
struct untracked_cache_dir *untracked,
- const char *dirname, int len, int exclude,
+ const char *dirname, int len, int baselen, int exclude,
const struct path_simplify *simplify)
{
/* The "len-1" is to strip the final '/' */
@@ -1324,7 +1324,8 @@ static enum path_treatment treat_directory(struct
dir_struct *dir,
if (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))
return exclude ? path_excluded : path_untracked;
- untracked = lookup_untracked(dir->untracked, untracked, dirname, len);
+ untracked = lookup_untracked(dir->untracked, untracked,
+dirname + baselen, len - baselen);
return read_directory_recursive(dir, dirname, len,
untracked, 1, simplify);
}
@@ -1444,6 +1445,7 @@ static int get_dtype(struct dirent *de, const char *path,
int len)
static enum path_treatment treat_one_path(struct dir_struct *dir,
struct untracked_cache_dir *untracked,
struct strbuf *path,
+ int baselen,
const struct path_simplify *simplify,
int dtype, struct dirent *de)
{
@@ -1495,8 +1497,8 @@ static enum path_treatment treat_one_path(struct
dir_struct *dir,
return path_none;
case DT_DIR:
strbuf_addch(path, '/');
- return treat_directory(dir, untracked, path->buf, path->len,
exclude,
- simplify);
+ return treat_directory(dir, untracked, path->buf, path->len,
+ baselen, exclude, simplify);
case DT_REG:
case DT_LNK:
return exclude ? path_excluded : path_untracked;
@@ -1557,7 +1559,7 @@ static enum path_treatment treat_path(struct dir_struct
*dir,
return path_none;
dtype = DTYPE(de);
- return treat_one_path(dir, untracked, path, simplify, dtype, de);
+ return treat_one_path(dir, untracked, path, baselen, simplify, dtype,
de);
}
static void add_untracked(struct untracked_cache_dir *dir, const char *name)
@@ -1827,7 +1829,7 @@ static int treat_leading_path(struct dir_struct *dir,
break;
if (simplify_away(sb.buf, sb.len, simplify))
break;
- if (treat_one_path(dir, NULL, &sb, simplify,
+ if (treat_one_path(dir, NULL, &sb, baselen, simplify,
DT_DIR, NULL) == path_none)
break; /* do not recurse into it */
if (len <= baselen) {
-- 8< --
> +static struct u
Re: [PATCH v3 1/4] refs: clean up common_list
On Thu, Aug 13, 2015 at 4:57 AM, David Turner wrote:
> +struct common_dir common_list[] = {
> + { "branches", 0, 1, 0 },
> + { "hooks", 0, 1, 0 },
> + { "info", 0, 1, 0 },
> + { "info/sparse-checkout", 0, 0, 1 },
> + { "logs", 1, 1, 0 },
> + { "logs/HEAD", 1, 1, 1 },
> + { "lost-found", 0, 1, 0 },
> + { "objects", 0, 1, 0 },
> + { "refs", 0, 1, 0 },
> + { "remotes", 0, 1, 0 },
> + { "worktrees", 0, 1, 0 },
> + { "rr-cache", 0, 1, 0 },
> + { "svn", 0, 1, 0 },
> + { "config", 0, 0, 0 },
> + { "gc.pid", 1, 0, 0 },
> + { "packed-refs", 0, 0, 0 },
> + { "shallow", 0, 0, 0 },
> + { NULL, 0, 0, 0 }
> };
Nit. If you make dirname the last field, we would have aligned
numbers, which might help reading.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/4] path: optimize common dir checking
On Thu, Aug 13, 2015 at 4:57 AM, David Turner wrote: > Instead of a linear search over common_list to check whether > a path is common, use a trie. The trie search operates on > path prefixes, and handles excludes. Just be careful that the given key from git_path is not normalized. I think you assume it is in the code, but I haven't read carefully. We could of course optimize for the good case: assume normalized and search, then fall back to explicit normalizing and search again. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] refs: make refs/worktree/* per-worktree
On Thu, Aug 13, 2015 at 4:57 AM, David Turner wrote: > We need a place to stick refs for bisects in progress that is not > shared between worktrees. So we use the refs/worktree/ hierarchy. > > The is_per_worktree_ref function and associated docs learn that > refs/worktree/ is per-worktree, as does the git_path code in path.c I assume you want to iterate over all these per-worktree refs later on. Otherwise just moving bisect refs outside refs/ would automatically make them per-worktree. But then it would polute $GIT_DIR some more, so probably not the best move. So yeah, refs/worktree is probably for the best. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 04/13] utf8: add function to align a string into given strbuf
On Sun, Aug 9, 2015 at 9:11 PM, Karthik Nayak wrote: > Add strbuf_utf8_align() which will align a given string into a strbuf > as per given align_type and width. If the width is greater than the > string length then no alignment is performed. I smell an opportunity to reuse this code and kill some in pretty.c's format_and_pad_commit(). If you want, you can do it. Else this is my personal note so I don't forget to do it later :) -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bug: git-archive does not use the zip64 extension for archives with more than 16k entries
On Thu, Aug 13, 2015 at 2:40 AM, René Scharfe wrote: > Seriously, though: What kind of repository has that many files and uses the > ZIP format to distribute snapshots? Just curious. Not the "uses the zip format" part, but at least webkit and gentoo-x86 both exceed 64k limit. Even if we don't support this case, we probably should error out instead of producing corrupt archives. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/17] removing questionable uses of git_path
On Mon, Aug 10, 2015 at 4:27 PM, Jeff King wrote: > The problem is that git_path uses a static buffer that gets overwritten > by subsequent calls. resolve_ref() was in the same boat, then we renamed it to resolve_ref_unsafe(), I believe with an intention to eventually kill it. But it lives on anyway. I wanted to suggest renaming git_path() to git_path_unsafe(). But I'm not sure if that will catch reviewer's eyes when new call sites are added. Probably not.. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: feature request: better support for typos
On Sat, Aug 8, 2015 at 1:12 AM, Ralf Thielow wrote: > Hi, > > when a user made a typo, Git is not good in guessing what > the user could have meant, except for git commands. I think > this is an area with room for improvements. > Let's look into branches. When I "clone --branch" and make > a typo, Git could show me what branch I could have meant. It's > the same when I try to merge or track a branch. Good candidate for those micro-projects next year. > It might even > be possible to show suggestions for options for all Git commands. You mean if you type "--brnch" it should suggest "--branch"? I was bugged about this and wanted to do something, only to realize in most cases git would show "git -h", which does a much better job because it would explain what --branch is for as well. > What I'm trying to say is, there are arguments with a limited > amount of possible values that Git know, so Git can show > suggestions when the user made a typo for such an argument. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re:
On Wed, Aug 5, 2015 at 7:47 PM, Ivan Chernyavsky wrote: > Dear community, > > For some time I'm wondering why there's no "--grep" option to the "git > branch" command, which would request to print only branches having specified > string/regexp in their history. Probably because nobody is interested and steps up to do it. The lack of response to you mail is a sign. Maybe you can try make a patch? I imagine it would not be so different from current --contains code, but this time we need to look into commits, not just commit id. > So for example: > > $ git branch -r --grep=BUG12345 > > should be roughly equivalent to following expression I'm using now for the > same task: > > $ for r in `git rev-list --grep=BUG12345 --remotes=origin`; do git branch > -r --list --contains=$r 'origin/*'; done | sort -u > > Am I missing something, is there some smarter/simpler way to do this? > > Thanks a lot in advance! > > -- > Ivan > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] untracked-cache: fix subdirectory handling
On Sun, Aug 16, 2015 at 12:17 PM, David Turner wrote:
> Previously, some calls lookup_untracked would pass a full path. But
> lookup_untracked assumes that the portion of the path up to and
> including to the untracked_cache_dir has been removed. So
> lookup_untracked would be looking in the untracked_cache for 'foo' for
> 'foo/bar' (instead of just looking for 'bar'). This would cause
> untracked cache corruption.
>
> Instead, treat_directory learns to track the base length of the parent
> directory, so that only the last path component is passed to
> lookup_untracked.
Your v2 also fixes untracked_cache_invalidate_path(), which is not
included here. Maybe it's in another patch?
> Helped-by: Nguyễn Thái Ngọc Duy
> Signed-off-by: David Turner
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
>
> This version incorporates Duy's version of the dir.c code, and his
> suggested test speedup.
>
> ---
> dir.c | 14
> t/t7063-status-untracked-cache.sh | 74
> +--
> 2 files changed, 80 insertions(+), 8 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index e7b89fe..cd4ac77 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1297,7 +1297,7 @@ static enum exist_status
> directory_exists_in_index(const char *dirname, int len)
> */
> static enum path_treatment treat_directory(struct dir_struct *dir,
> struct untracked_cache_dir *untracked,
> - const char *dirname, int len, int exclude,
> + const char *dirname, int len, int baselen, int exclude,
> const struct path_simplify *simplify)
> {
> /* The "len-1" is to strip the final '/' */
> @@ -1324,7 +1324,8 @@ static enum path_treatment treat_directory(struct
> dir_struct *dir,
> if (!(dir->flags & DIR_HIDE_EMPTY_DIRECTORIES))
> return exclude ? path_excluded : path_untracked;
>
> - untracked = lookup_untracked(dir->untracked, untracked, dirname, len);
> + untracked = lookup_untracked(dir->untracked, untracked,
> +dirname + baselen, len - baselen);
> return read_directory_recursive(dir, dirname, len,
> untracked, 1, simplify);
> }
> @@ -1444,6 +1445,7 @@ static int get_dtype(struct dirent *de, const char
> *path, int len)
> static enum path_treatment treat_one_path(struct dir_struct *dir,
> struct untracked_cache_dir
> *untracked,
> struct strbuf *path,
> + int baselen,
> const struct path_simplify
> *simplify,
> int dtype, struct dirent *de)
> {
> @@ -1495,8 +1497,8 @@ static enum path_treatment treat_one_path(struct
> dir_struct *dir,
> return path_none;
> case DT_DIR:
> strbuf_addch(path, '/');
> - return treat_directory(dir, untracked, path->buf, path->len,
> exclude,
> - simplify);
> + return treat_directory(dir, untracked, path->buf, path->len,
> + baselen, exclude, simplify);
> case DT_REG:
> case DT_LNK:
> return exclude ? path_excluded : path_untracked;
> @@ -1557,7 +1559,7 @@ static enum path_treatment treat_path(struct dir_struct
> *dir,
> return path_none;
>
> dtype = DTYPE(de);
> - return treat_one_path(dir, untracked, path, simplify, dtype, de);
> + return treat_one_path(dir, untracked, path, baselen, simplify, dtype,
> de);
> }
>
> static void add_untracked(struct untracked_cache_dir *dir, const char *name)
> @@ -1827,7 +1829,7 @@ static int treat_leading_path(struct dir_struct *dir,
> break;
> if (simplify_away(sb.buf, sb.len, simplify))
> break;
> - if (treat_one_path(dir, NULL, &sb, simplify,
> + if (treat_one_path(dir, NULL, &sb, baselen, simplify,
>DT_DIR, NULL) == path_none)
> break; /* do not recurse into it */
> if (len <= baselen) {
> diff --git a/t/t7063-status-untracked-cache.sh
> b/t/t7063-status-untracked-cache.sh
> index ff23f4e..6716f69 100755
> --- a/t/t7063-status-untracked-cache.sh
> +++ b/t/t7063-status-untracked-cache.sh
> @@ -402,13 +402,14 @@ test_expect_success 'set up sparse checkout' '
> echo "done/[a-z]*" >.git/info/sparse-checkout &&
> test_config core.sparsecheckout true &&
> git checkout master &&
> - git update-index --untracked-cache &&
> + git update-index --untracked-cache --force-untracked-cache &&
> git status --porcelain >/dev/null && # prime the cache
> test_path_is_missing done/.gitignore &&
> test_path_is_file done/one
> '
>
> -test_expect_success 'create files, some
Re: [PATCH v3 2/4] path: optimize common dir checking
On Sun, Aug 16, 2015 at 12:04 PM, David Turner wrote:
> Duy Nguyen writes:
>> On Thu, Aug 13, 2015 at 4:57 AM, David Turner
> wrote:
>> > Instead of a linear search over common_list to check whether
>> > a path is common, use a trie. The trie search operates on
>> > path prefixes, and handles excludes.
>>
>> Just be careful that the given key from git_path is not normalized. I
>> think you assume it is in the code, but I haven't read carefully. We
>> could of course optimize for the good case: assume normalized and
>> search, then fall back to explicit normalizing and search again.
>
> What does it mean for a key to be normalized? Do you mean normalized in
> terms of upper/lowercase on case-insensitive filesystems? If so, I think the
> assumption here is that this will be called with paths generated by git,
> which will always use the lowercase path.
Mostly about duplicated slashes, "abc//def" instead of "abc/def".
Technically nothing forbids git_path("refs/../refs/foo"), but that
would fool even current code.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] untracked-cache: fix subdirectory handling
On Sun, Aug 16, 2015 at 7:16 PM, Duy Nguyen wrote: > On Sun, Aug 16, 2015 at 12:17 PM, David Turner > wrote: >> Previously, some calls lookup_untracked would pass a full path. But >> lookup_untracked assumes that the portion of the path up to and >> including to the untracked_cache_dir has been removed. So >> lookup_untracked would be looking in the untracked_cache for 'foo' for >> 'foo/bar' (instead of just looking for 'bar'). This would cause >> untracked cache corruption. >> >> Instead, treat_directory learns to track the base length of the parent >> directory, so that only the last path component is passed to >> lookup_untracked. > > Your v2 also fixes untracked_cache_invalidate_path(), which is not > included here. Maybe it's in another patch? No I was wrong. Your changes and the original code are effectively the same (I misread strrchr as strchr). But I think there's a bug somewhere as I'm writing tests to understand that code.. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] write_file(): introduce an explicit WRITE_FILE_GENTLY request
On Tue, Aug 25, 2015 at 1:41 AM, Junio C Hamano wrote: > Junio C Hamano writes: > >> All callers except for two ask this function to die upon error by >> passing fatal=1; turn the parameter to a more generic "unsigned flag" >> bag of bits, introduce an explicit WRITE_FILE_GENTLY bit and change >> these two callers to pass that bit. > > There is a huge iffyness around one of these two oddball callers. > >> diff --git a/setup.c b/setup.c >> index 5f9f07d..718f4e1 100644 >> --- a/setup.c >> +++ b/setup.c >> @@ -404,7 +404,7 @@ static void update_linked_gitdir(const char *gitfile, >> const char *gitdir) >> >> strbuf_addf(&path, "%s/gitfile", gitdir); >> if (stat(path.buf, &st) || st.st_mtime + 24 * 3600 < time(NULL)) >> - write_file(path.buf, 0, "%s\n", gitfile); >> + write_file(path.buf, WRITE_FILE_GENTLY, "%s\n", gitfile); >> strbuf_release(&path); >> } > > This comes from 23af91d1 (prune: strategies for linked checkouts, > 2014-11-30). I cannot tell what the justification is to treat a > failure to write a gitfile as a non-error event. Just a sloppy > coding that lets the program go through to its finish, ignoring the > harm done by possibly corrupting user repository silently? Failing to write to this file is not a big deal _if_ the file is not corrupted because of this write operation. But we should not be so silent about this. If the file content is corrupted and it's old enough, this checkout may be pruned. I think there's another bug here... wrong name.. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Unable to create temporary file '/var/git/tmv3-target-overlay.git/shallow_Un8ZOR': Permission denied
On Fri, Aug 21, 2015 at 6:36 PM, Joakim Tjernlund wrote: > I cannot push: > # > git push origin > Login for jo...@git.transmode.se > Password: > Counting objects: 7, done. > Delta compression using up to 4 threads. > Compressing objects: 100% (7/7), done. > Writing objects: 100% (7/7), 13.73 KiB | 0 bytes/s, done. > Total 7 (delta 4), reused 0 (delta 0) > fatal: Unable to create temporary file > '/var/git/tmv3-target-overlay.git/shallow_Un8ZOR': Permission denied > fatal: The remote end hung up unexpectedly > fatal: The remote end hung up unexpectedly Noted. Will try to fix (but probably not fast). At first I thought this was an old bug, but that old bug [1] is in the fetch/clone path, not push. Not sure if the same approach can be reused here (i.e.avoid temp files altoghether). [1] b790e0f (upload-pack: send shallow info over stdin to pack-objects - 2014-03-11) -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] commit: don't rewrite shared index unnecessarily
On Fri, Aug 28, 2015 at 12:07 AM, David Turner wrote: > Remove a cache invalidation which would cause the shared index to be > rewritten on as-is commits. > > When the cache-tree has changed, we need to update it. But we don't > necessarily need to update the shared index. So setting > active_cache_changed to SOMETHING_CHANGED is unnecessary. Instead, we > let update_main_cache_tree just update the CACHE_TREE_CHANGED bit. > > In order to test this, make test-dump-split-index not segfault on > missing replace_bitmap/delete_bitmap. This new codepath is not called > now that the test passes, but is necessary to avoid a segfault when the > new test is run with the old builtin/commit.c code. > > Signed-off-by: David Turner Ack. I made SOMETHING_CHANGED "1" for catching these cases (there were a few on-flight topics when this series was being cooked and I was afraid I could not cache all active_cache_changed sites). > --- > > I introduced this bug last year while improving the cache-tree code. > I guess I probably didn't notice that active_cache_changed wasn't a > boolean. So.. you did you split-index? Cool. Never heard anyone using it for real. It needs the other part to improve reading/refresh side to get to full potential though.. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git's inconsistent command line options
On Wed, Aug 26, 2015 at 6:43 AM, Junio C Hamano wrote: > ... > > I do not see a good way to do such a safe transition with command > words approach, *unless* we are going to introduce new commands, > i.e. "git list-tag", "git create-tag", etc. I'm probably shot down for this. But could we go with a clean plate and create a new command prefix (something like git-next, git2, or gt...)? We could then redesign the entire UI without worrying about backward compatibility. At some point we can start to deprecate "git" and encourage to use the new command prefix only. Of course somebody has to go over all the commands and options to propose some consistent UI, then more discussions and coding so it could likely follow the path of pack v4.. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] dir.c: make last_exclude_matching_from_list() run til the end
On Wed, Aug 26, 2015 at 3:28 AM, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > >> Signed-off-by: Nguyễn Thái Ngọc Duy > > Because? Title just tells what the patch meant to do (i.e. instead > of returning it keeps looping), but does not say why it is a good > idea. Besides, this a no-op patch and does not make it keep looping. Because the next patch adds some post processing before returning the value. Having all the paths come to the same point would simplify the code. Will update the commit message. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] gc: save log from daemonized gc --auto and print it next time
On Wed, Aug 26, 2015 at 12:49 AM, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > >> While commit 9f673f9 (gc: config option for running --auto in >> background - 2014-02-08) helps reduce some complaints about 'gc >> --auto' hogging the terminal, it creates another set of problems. >> >> The latest in this set is, as the result of daemonizing, stderr is >> closed and all warnings are lost. This warning at the end of cmd_gc() >> is particularly important because it tells the user how to avoid "gc >> --auto" running repeatedly. Because stderr is closed, the user does >> not know, naturally they complain about 'gc --auto' wasting CPU. >> >> Daemonized gc now saves stderr to $GIT_DIR/gc.log. Following gc runs >> will not be daemonized and gc.log printed out until the user removes >> gc.log. >> >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- >> Let's try again. Compared to v2 [1], this version does not delete >> gc.log automatically any more. The user needs to take action, then >> delete gc.log to bring it background again. > > Sounds a bit more sensible, but I wonder if it is a good idea to > keep going in the first place. If the last gc run reported an > issue, would it make it likely that we would hit the same issue? > > An alternative design I have in mind is to exit "gc --auto" with > success without doing anything, after giving the new warning > message. What would be the pros-and-cons between this patch and > that alternative design? I think the alt. design is better. If anything, keep runing may produce more output and probably distract the user from the real warning. We only want to keep doing something again if it can gain us something. If we succeed at steps A and B, then fail at C. Doing A and B again next time might be worth something in general, but given the context of git-gc I don't think it is. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8] diff.h: extend "flags" field to 64 bits because we're out of bits
On Wed, Aug 26, 2015 at 12:39 AM, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > >> I renamed both "flags" and "touched_flags" fields while making this >> patch to make sure I was aware of how these flags were manipulated >> (besides DIFF_OPT* macros). So hopefully I didn't miss anything. > > It is a bad taste to use user_defined_t typedef (I think it actually > is a standard violation), isn't it? Yeah I think you posted a patch somewhere updating CodingGuidelines about this.. > The diff-struct is not like objects where we need million copies of > in-core while running. What do you need many more flags for? We already use all 32 bit flags and I need one more flag. I guess I go with flags because it's how we add features in diff struct. Adding a new field instead of extending flags could be dangerous: elsewhere people copy flags out to a temporary place, do something then restore. If it's a separate field, it's left in place and bad things could happen. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] setup: update the right file in multiple checkouts
On Tue, Aug 25, 2015 at 11:38 PM, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > >> This code is introduced in 23af91d (prune: strategies for linked >> checkouts - 2014-11-30), and it's supposed to implement this rule from >> that commit's message: >> >> - linked checkouts are supposed to keep its location in $R/gitdir up >>to date. The use case is auto fixup after a manual checkout move. >> >> Note the name, "$R/gitdir", not "$R/gitfile". Correct the path to be >> updated accordingly. >> >> While at there, make sure I/O errors are not silently dropped. >> >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- >> The code was right in v2 [1] and became "gitfile" since v3 [2]. I >> need to reconsider my code quality after this :( > > Heh, don't sweat it. Everybody makes mistakes and sometimes becomes > sloppy. > > Thanks for double checking and correcting. Perhaps this could have > caught if we had some test coverage, I wonder? There's tests to check this prune functionality. They just don't exercise this function. Instead they manipulate "gitdir" file directly. I'll add a test to move repo around to make sure this code is exercised in the test suite. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/5] enter_repo: avoid duplicating logic, use is_git_directory() instead
On Mon, Sep 7, 2015 at 11:33 PM, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > >> Signed-off-by: Nguyễn Thái Ngọc Duy >> --- > > The cover letter talks about "local clone", and in this entire > series, I saw new tests only for the local case, but doesn't this > and the next change also affect the case where a Git daemon or a > upload-pack process is serving the remote repository? > > And if so, how is that case affected? People who serve .git-dir repos should not be affected (I think we have enough test cases covering that). People can serve .git-file repos as well, which is sort of tested in the local clone test case because upload-pack is involved for providing remote refs, I think. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/5] enter_repo: avoid duplicating logic, use is_git_directory() instead
On Sun, Sep 13, 2015 at 8:04 AM, Junio C Hamano wrote: > Duy Nguyen writes: > >> On Mon, Sep 7, 2015 at 11:33 PM, Junio C Hamano wrote: >>> Nguyễn Thái Ngọc Duy writes: >>> >>>> Signed-off-by: Nguyễn Thái Ngọc Duy >>>> --- >>> >>> The cover letter talks about "local clone", and in this entire >>> series, I saw new tests only for the local case, but doesn't this >>> and the next change also affect the case where a Git daemon or a >>> upload-pack process is serving the remote repository? >>> >>> And if so, how is that case affected? >> >> People who serve .git-dir repos should not be affected (I think we >> have enough test cases covering that). People can serve .git-file >> repos as well, which is sort of tested in the local clone test case >> because upload-pack is involved for providing remote refs, I think. > > Unfortunately, the above is still not unclear to me. > > Was serving from a linked repository working without these five > patches, i.e. was the local case the only one that was broken and > needed fixing with these five patches? If so, the log message > should mention that (i.e. "remote case was working OK but local was > broken because ...; change this and that to make local one work as > well"). If the remote case also was broken and fixed by these five > patches, then that is also worth mentioning the same way. > > I didn't ask you to explain it to me in the first place in a > response. The review comment pointed out that the proposed log > message was unclear and those who will be reading "git log" output > need clearer description. I know. I sent the re-roll before receiving this. I think I still haven't mentioned the impact on remote case. Another update coming, maybe next weekend. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git rebase -i failing due to phantom index.lock
On Thu, Sep 17, 2015 at 3:08 AM, Giuseppe Bilotta
wrote:
> Hello all,
>
> I've recently started to note an issue with git rebase -i failing with
> alarming frequency, especially on one of my repositories, claiming
> that index.lock could not be created because it exists, even though it
> doesn't and nothing else seems to be locking the index. The rebase
> bombs more usually during the initial checkout than during any other
> of the steps.
>
> The problem is somewhat randomly reproducible, as it doesn't happen
> 100% of the time. It also seems to happen more consistently with more
> recent git versions, or at least with my custom built git than with
> the distro-shipped one.
>
> A somewhat problematic git bisect has allowed me to identify commit
> 03b86647722f11ccc321cd7279aa49b811d17cc2 as the first bad commit.
That commit is mostly refactoring, but there's one extra lock added in
these hunks. Maybe you can revert it and see if it improves anything.
diff --git a/builtin/apply.c b/builtin/apply.c
index 87439fa..5e13444 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3644,7 +3644,7 @@ static void build_fake_ancestor(struct patch
*list, const char *filename)
{
struct patch *patch;
struct index_state result = { NULL };
- int fd;
+ static struct lock_file lock;
/* Once we start supporting the reverse patch, it may be
* worth showing the new sha1 prefix, but until then...
@@ -3682,8 +3682,8 @@ static void build_fake_ancestor(struct patch
*list, const char *filename)
die ("Could not add %s to temporary index", name);
}
- fd = open(filename, O_WRONLY | O_CREAT, 0666);
- if (fd < 0 || write_index(&result, fd) || close(fd))
+ hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);
+ if (write_locked_index(&result, &lock, COMMIT_LOCK))
die ("Could not write temporary index to %s", filename);
discard_index(&result);
> The problem has all signs of a timing issue, which I'm having problems
> isolating, although simply providing a timeout on the index lock calls
> seems to fix it.
lockfile has received some updates lately. This commit caught my eyes,
044b6a9 (lockfile: allow file locking to be retried with a timeout -
2015-05-11), but this is just a shot in the dark..
> Making git stall instead of die on error allowed me
> to obtain this backtrace which I suspect will not be particularly
> useful:
>
> #0 0x004c4666 in unable_to_lock_message
> (path=path@entry=0x1bad940 ".git/index", err=,
> buf=buf@entry=0x7ffd3b990900) at lockfile.c:158
> #1 0x004c46c6 in unable_to_lock_die (path=path@entry=0x1bad940
> ".git/index", err=) at lockfile.c:167
> #2 0x004c480b in hold_lock_file_for_update_timeout
> (lk=lk@entry=0x1bd7be0, path=0x1bad940 ".git/index", flags= out>, timeout_ms=timeout_ms@entry=0) at lockfile.c:177
> #3 0x004e6e2a in hold_lock_file_for_update (flags=1,
> path=, lk=0x1bd7be0) at lockfile.h:165
> #4 hold_locked_index (lk=lk@entry=0x1bd7be0,
> die_on_error=die_on_error@entry=1) at read-cache.c:1411
> #5 0x00420cb0 in merge_working_tree (old=0x7ffd3b990a20,
> old=0x7ffd3b990a20, new=0x7ffd3b990a00, new=0x7ffd3b990a00,
> writeout_error=0x7ffd3b9909c0, opts=0x7ffd3b992a31)
> at builtin/checkout.c:471
So it fails at "git checkout". That'll give me something to look in
git-rebase*.sh. Thanks.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] gc: save log from daemonized gc --auto and print it next time
On Wed, Sep 16, 2015 at 11:00 PM, Junio C Hamano wrote: > Michael Haggerty writes: > >> I'm not sure what behavior you want. At one point you say "puts the file >> to a final place if it is non-empty" but later you say "leave it if >> non-empty". Should the file be written directly, or should it be written >> to a lockfile and renamed into place only when complete? > > I do not think we care that deeply either way, as we do not want to > run multiple auto-gc's at the same time in the first place. So either > one of the following is perfectly fine: > > * We open the final destination directly, but with O_CREAT|O_EXCL, >which as a side effect also detects the simultanous execution >[*1*]. We do not do any "finalizing rename" if we go this route. >When we are done, close it and remove it if we did not write >anything, or leave it there if we did write something. > > * We open a lockfile the usual way. When we are done, close it and >and remove it if we did not write anything, or finalize it by >renaming it if we did write something. > > I think Duy's code wants to do the latter. We do keep another lock before coming to opening this log file. So once we get here we already know nobody else will be opening the log file. We can simply open it the normal way, then make sure we clean it up at atexit(). >> This doesn't seem like a common thing to want (as in, this might be the >> only caller), but it probably makes sense to build it into the >> tempfile/lockfile API nevertheless, because implementing it externally >> would require a lot of other code to be duplicated. >> >> Another possibility that might work (maybe without requiring changes to >> tempfile/lockfile): don't worry about deleting the log file if it is >> empty, but make observers treat an empty log file the same as an absent one. > > Probably your "don't remove and check for emptiness" approach would > be the simpler of the two, but I think we can go either way. People have complained to me about stray files in $GIT_DIR, so it's probably best that we delete empty/useless files. Although we could delete empty files at the beginning of the next gc instead of at atexit(). Let me try it out and see which is simplest. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Unable to create temporary file '/var/git/tmv3-target-overlay.git/shallow_Un8ZOR': Permission denied
On Mon, Sep 14, 2015 at 10:37 PM, Joakim Tjernlund
wrote:
> On Mon, 2015-08-31 at 16:56 +0700, Duy Nguyen wrote:
>> On Fri, Aug 21, 2015 at 6:36 PM, Joakim Tjernlund
>> wrote:
>> > I cannot push:
>> > # > git push origin
>> > Login for jo...@git.transmode.se
>> > Password:
>> > Counting objects: 7, done.
>> > Delta compression using up to 4 threads.
>> > Compressing objects: 100% (7/7), done.
>> > Writing objects: 100% (7/7), 13.73 KiB | 0 bytes/s, done.
>> > Total 7 (delta 4), reused 0 (delta 0)
>> > fatal: Unable to create temporary file
>> > '/var/git/tmv3-target-overlay.git/shallow_Un8ZOR': Permission
>> > denied
>> > fatal: The remote end hung up unexpectedly
>> > fatal: The remote end hung up unexpectedly
>>
>> Noted. Will try to fix (but probably not fast). At first I thought
>> this was an old bug, but that old bug [1] is in the fetch/clone path,
>> not push. Not sure if the same approach can be reused here (i.e.avoid
>> temp files altoghether).
>>
>> [1] b790e0f (upload-pack: send shallow info over stdin to pack-objects
>> - 2014-03-11)
>
> Noticed I had forgotten to reply ...
>
> An even simpler fix would be to have an tmp dir within the repo, aka:
> /var/git/tmv3-target-overlay.git/tmp/shallow_Un8ZOR
> This would cover all cases when one must create a tmp file
Sorry for my silence. I intend to put these temp files in $TMPDIR by
resurrecting (part of) this patch [1]. Maybe tomorrow.
But if you build your own, you can put them in $GIT_DIR/tmp by
replacing "shallow_XX" in setup_temporary_shallow() in shallow.c
with "tmp/shallow_". You need to create the directory "tmp" in
advance though, or do
"safe_create_leading_directories_const(git_path("tmp/shallow_X"));"
before xmkstemp()
[1] http://article.gmane.org/gmane.comp.version-control.git/242787
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] dir.c: don't exclude whole dir prematurely if neg pattern may match
On Tue, Sep 15, 2015 at 12:15 AM, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > >> diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt >> index 473623d..889a72a 100644 >> --- a/Documentation/gitignore.txt >> +++ b/Documentation/gitignore.txt >> @@ -82,12 +82,9 @@ PATTERN FORMAT >> >> - An optional prefix "`!`" which negates the pattern; any >> matching file excluded by a previous pattern will become >> + included again. It is possible to re-include a file if a parent >> + directory of that file is excluded, with restrictions. See section >> + NOTES for detail. > > Sounds like a very useful thing. > >> - If the pattern ends with a slash, it is removed for the >> purpose of the following description, but it would only find >> @@ -141,6 +138,18 @@ not tracked by Git remain untracked. >> To stop tracking a file that is currently tracked, use >> 'git rm --cached'. >> >> +To re-include a file when its parent directory is excluded, the >> +following conditions must be met: >> + >> + - The directory part in the re-include rules must be literal (i.e. no >> + wildcards) >> + >> + - The rules to exclude the parent directory must not end with a >> + trailing slash. >> + >> + - The rules to exclude the parent directory must have at least one >> + slash. >> + > > In this bulletted list, don't the readers also need to be told that > having "/abc" in .gitignore (but not "!/abc/anything" in .gitignore) > and "!foo" in abc/.gitignore would not cause us to descend into > "/abc" just to examine "abc/.gitignore" with pessimistic assumption > that there might be some exclusion in there? Yeah I thought it was already mentioned. I guess it's just in my mind. Will update. >> diff --git a/t/t3001-ls-files-others-exclude.sh >> b/t/t3001-ls-files-others-exclude.sh >> index 3fc484e..9de49a6 100755 >> --- a/t/t3001-ls-files-others-exclude.sh >> +++ b/t/t3001-ls-files-others-exclude.sh >> @@ -305,4 +305,24 @@ test_expect_success 'ls-files with "**" patterns and no >> slashes' ' >> test_cmp expect actual >> ' >> >> +test_expect_success 'negative patterns' ' >> + git init reinclude && >> + ( >> + cd reinclude && >> + cat >.gitignore <<-\EOF && >> + /foo >> + !foo/bar/bar >> + EOF >> + mkdir -p foo/bar && >> + touch abc foo/def foo/bar/ghi foo/bar/bar && >> + git ls-files -o --exclude-standard >../actual && >> + cat >../expected <<-\EOF && >> + .gitignore >> + abc >> + foo/bar/bar >> + EOF >> + test_cmp ../expected ../actual >> + ) >> +' > > And another test here may want to explicitly ensure that we are not > overly pessimising the ignore processing, so that later changes will > not break it, I think. Yep. Will do. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: On a personal note
On Thu, Sep 3, 2015 at 5:00 PM, Johannes Schindelin wrote: > Hey all, > > yes, it is true: since mid-August I am working for Microsoft. Over a > year ago, I got into contact with the Visual Studio Online group at > Microsoft, of which I am now a happy member. A large part of my mission > is to improve the experience of Git for Windows. This is very exciting > to me: I finally can focus pretty much full time on something that I > could only address in my spare time previously. Is upstreaming msysgit-specific patches in the roadmap? It would be very nice to have everything in one tree (or at least keep the two trees as close as possible). -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Unable to create temporary file '/var/git/tmv3-target-overlay.git/shallow_Un8ZOR': Permission denied
On Thu, Sep 17, 2015 at 11:54 PM, Joakim Tjernlund wrote: > On Thu, 2015-09-17 at 20:18 +0700, Duy Nguyen wrote: >> On Mon, Sep 14, 2015 at 10:37 PM, Joakim Tjernlund >> wrote: >> > On Mon, 2015-08-31 at 16:56 +0700, Duy Nguyen wrote: >> > > On Fri, Aug 21, 2015 at 6:36 PM, Joakim Tjernlund >> > > wrote: >> > > > I cannot push: >> > > > # > git push origin >> > > > Login for jo...@git.transmode.se >> > > > Password: >> > > > Counting objects: 7, done. >> > > > Delta compression using up to 4 threads. >> > > > Compressing objects: 100% (7/7), done. >> > > > Writing objects: 100% (7/7), 13.73 KiB | 0 bytes/s, done. >> > > > Total 7 (delta 4), reused 0 (delta 0) >> > > > fatal: Unable to create temporary file >> > > > '/var/git/tmv3-target-overlay.git/shallow_Un8ZOR': Permission >> > > > denied I'm about to do it, but now I'm not sure if I should move shallow_XX out of $GIT_DIR. It will not be the only command that may write to $GIT_DIR. "git gc --auto" (which can be triggered at the server side at push time) can write $GIT_DIR/gc.pid (and soon, gc.log). Even if you disable gc --auto and run it periodically (with cron or something), it will write gc.pid. Is it really necessary to remove write access in $GIT_DIR? Do we (git devs) have some guidelines about things in $GIT_DIR? -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Unable to create temporary file '/var/git/tmv3-target-overlay.git/shallow_Un8ZOR': Permission denied
On Sat, Sep 19, 2015 at 9:21 AM, Duy Nguyen wrote: > Even if you disable gc --auto and run it periodically (with > cron or something), it will write gc.pid. Ignore this sentence. Of course you can run manual gc using a different user and with write access. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Single brackets matching in .gitignore rules
On Sun, Sep 27, 2015 at 5:01 AM, Andrey Loskutov wrote: > ... > Anyway, it would be nice to hear what should be the "right" way to interpret > the tables above. > > BTW the only official documentation I found about ignore rules: > > https://www.kernel.org/pub/software/scm/git/docs/gitignore.html > http://man7.org/linux/man-pages/man3/fnmatch.3.html > http://man7.org/linux/man-pages/man7/glob.7.html This is already answered. I just want to add that C Git has stopped using system fnmatch for some time (part of the reason is system fnmatch behaves differently in corner cases). If you don't mind C, have a look at dowild() in wildmatch.c, or t/t3070-wildmatch.sh for some corner cases (but your cases aren't there, may be worth adding too) -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-new-workdir: add windows compatibility
On Fri, May 29, 2015 at 4:53 PM, Michael J Gruber wrote: > Isn't that basically the approach that "git checkout --to" is taking? Is > that one "Windows proof"? It should be. > I've lost track of its status, though. It should be fine to use as long as you don't do checkout --to on a submodule. Some progress was made on that direction, but I was busy with other series and then didn't have time for git for a while. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git clone --depth: shallow clone problems
On Tue, May 26, 2015 at 9:54 PM, Junio C Hamano wrote:
> Duy Nguyen writes:
>
>> --unshallow::
>> - If the source repository is complete, convert a shallow
>> - repository to a complete one, removing all the limitations
>> + If the source repository is complete, convert all shallow
>> + refs to complete ones, removing all the limitations
>
> Define "shallow ref", or better yet explain without introducing such
> a new term (I do not think shallow-ness is a property of a ref, by
> the way---I think you meant all refs that can reach the points the
> history is cauterised by .git/shallow, but we shouldn't assume that
> the target audience of this paragraph to know Git as well as I do).
OK maybe
Make sure all existing refs have the same history as the ones from the
source repository. If the source repository is complete, this removes
all limitations imposed by shallow repositories.
?
>> imposed by shallow repositories.
>> +
>> If the source repository is shallow, fetch as much as possible so that
>> -the current repository has the same history as the source repository.
>> +the all existing refs of current repository have the same history as in
>> +the source repository.
>
> Makes sense, modulo "so that the all existing refs" sounds strange
> to my ears ("all the existing refs" perhaps).
Will fix. Although I think the new paragraph above already covers this case too.
>> ++
>> +Note that if the repository is created with --single-branch, which is
>> +default for a shallow clone, only one ref is completed. `--unshallow`
>> +does not fetch all remaining refs from source repository.
>
> I do not think this "Note" is being as helpful as it could be.
>
> - If the repository was created with --single-branch but if the
>user later fetched and started tracking other branches, the
>statement "only one ref is completed" is untrue; the true version
>is "only the existing refs are completed", which leads to a more
>important point. It says the same thing as "all existing refs"
>above and does not add any useful information.
>
> - The last sentence is less than useful but merely frustrating---it
>says what you cannot do with this option, strongly hints that the
>writer of the sentence knows what the reader wants to achieve,
>but without saying what other way the reader go to achieve it.
>
> Perhaps replace that Note paragraph with something along this line?
>
> +
> By fetching and tracking refs that you haven't been tracking,
> you can add these new refs to "all refs" to be unshallowed.
It was meant to emphasize --unshallow would not create a true clone,
if people miss the keywords "all existing" refs.. Maybe..
You may need to fetch and track other refs from the source repository
if you want to make full clone.
?
>
>>> 2) git fetch --unshallow should convert the clone into an actual
>>> equivalent of a normal, not shallow clone.
>>
>> I was thinking of some way to undo --single-branch too. I don't think
>> it should be the job of --unshallow, maybe a new option, but I can't
>> think of a name better than --really-unshallow :P
>
> Isn't that just the matter of updating remote.origin.fetch? I do
> not think it belongs to "clone" (or "fetch"). Perhaps it belongs to
> "remote", where it already shows with "git remote -v" what is
> fetched and pushed.
Yeah, perhaps showing an advice about git-remote is enough if the
current ref spec does not fetch everything.
>> --depth ::
>> Create a 'shallow' clone with a history truncated to the
>> - specified number of revisions.
>> + specified number of revisions. --single-branch is
>> + automatically specified unless the user overrides it with
>> + --no-single-branch
>
> Yeah, something like that would be a definite improvement.
>
> By the way, while composing this message, I scratched my head after
> typing
>
> $ git clone --shallow=4 --no-local ./git.git ./playpen
>
> Is it just me or do we want to add such a synonym?
I have a series to define shallow boundaries by tags or dates, in
addition to history depth. It may take a few more months until I'm
finished with my ongoing topics. Then you could think about what
--shallow means exactly ;-)
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] checkout: don't check worktrees when not necessary
On Sun, May 31, 2015 at 07:16:29PM -0400, Spencer Baugh wrote:
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1237,6 +1237,7 @@ static int parse_branchname_arg(int argc, const char
> **argv,
> char *head_ref = resolve_refdup("HEAD", 0, sha1, &flag);
> if (head_ref &&
> (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)) &&
> + !(opts->patch_mode || opts->pathspec.nr) &&
> !opts->ignore_other_worktrees)
> check_linked_checkouts(new);
> free(head_ref);
Simple and effective. But if in future we add more options for
non-switching-branch checkout, we need to update both places, here and
near the end of cmd_checkout().
Perhaps we can move all this block inside checkout_branch() so we only
need to test "opts->patch_mode || opts->pathspec.nr" once, at the end
of cmd_checkout(). Something like below?
I'm not opposed to your change, but if you go with it, you should
cherry pick my test in the below patch. Or create a similar test.
-- 8< --
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2f92328..e9aee58 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1110,7 +1110,6 @@ static int parse_branchname_arg(int argc, const char
**argv,
{
struct tree **source_tree = &opts->source_tree;
const char **new_branch = &opts->new_branch;
- int force_detach = opts->force_detach;
int argcount = 0;
unsigned char branch_rev[20];
const char *arg;
@@ -1231,17 +1230,6 @@ static int parse_branchname_arg(int argc, const char
**argv,
else
new->path = NULL; /* not an existing branch */
- if (new->path && !force_detach && !*new_branch) {
- unsigned char sha1[20];
- int flag;
- char *head_ref = resolve_refdup("HEAD", 0, sha1, &flag);
- if (head_ref &&
- (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)) &&
- !opts->ignore_other_worktrees)
- check_linked_checkouts(new);
- free(head_ref);
- }
-
new->commit = lookup_commit_reference_gently(rev, 1);
if (!new->commit) {
/* not a commit */
@@ -1321,6 +1309,17 @@ static int checkout_branch(struct checkout_opts *opts,
die(_("Cannot switch branch to a non-commit '%s'"),
new->name);
+ if (new->path && !opts->force_detach && !opts->new_branch) {
+ unsigned char sha1[20];
+ int flag;
+ char *head_ref = resolve_refdup("HEAD", 0, sha1, &flag);
+ if (head_ref &&
+ (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)) &&
+ !opts->ignore_other_worktrees)
+ check_linked_checkouts(new);
+ free(head_ref);
+ }
+
if (opts->new_worktree)
return prepare_linked_checkout(opts, new);
diff --git a/t/t2025-checkout-to.sh b/t/t2025-checkout-to.sh
index f8e4df4..a8d9336 100755
--- a/t/t2025-checkout-to.sh
+++ b/t/t2025-checkout-to.sh
@@ -28,6 +28,14 @@ test_expect_success 'checkout --to refuses to checkout
locked branch' '
! test -d .git/worktrees/zere
'
+test_expect_success 'checking out paths not complaining about linked
checkouts' '
+ (
+ cd existing_empty &&
+ echo dirty >>init.t &&
+ git checkout master -- init.t
+ )
+'
+
test_expect_success 'checkout --to a new worktree' '
git rev-parse HEAD >expect &&
git checkout --detach --to here master &&
-- 8< --
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFCv2 01/16] stringlist: add from_space_separated_string
On Tue, Jun 2, 2015 at 7:02 AM, Stefan Beller wrote:
> diff --git a/string-list.h b/string-list.h
> index d3809a1..88c18e9 100644
> --- a/string-list.h
> +++ b/string-list.h
> @@ -19,6 +19,7 @@ struct string_list {
> #define STRING_LIST_INIT_DUP { NULL, 0, 0, 1, NULL }
>
> void string_list_init(struct string_list *list, int strdup_strings);
> +void from_space_separated_string(struct string_list *list, char *line);
The name feels out of place. All functions in here have "string_list"
somewhere in their names. The implementation looks very close to
string_list_split() but that name's already taken.. Maybe
string_list_split_by_space()?
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFCv2 13/16] fetch-pack: use the configured transport protocol
On Tue, Jun 2, 2015 at 7:02 AM, Stefan Beller wrote:
> @@ -127,6 +128,10 @@ int cmd_fetch_pack(int argc, const char **argv, const
> char *prefix)
> args.update_shallow = 1;
> continue;
> }
> + if (!skip_prefix(arg, "--transport-version", &arg)) {
I think this line should be "if (skip_prefix(arg,
"--transport-version=", &arg)) {".
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFCv2 10/16] transport: connect_setup appends protocol version number
On Tue, Jun 2, 2015 at 7:02 AM, Stefan Beller wrote:
> Signed-off-by: Stefan Beller
> ---
>
> Notes:
> name it to_free
>
> transport.c | 17 +++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/transport.c b/transport.c
> index 651f0ac..b49fc60 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -496,15 +496,28 @@ static int set_git_option(struct git_transport_options
> *opts,
> static int connect_setup(struct transport *transport, int for_push, int
> verbose)
> {
> struct git_transport_data *data = transport->data;
> + const char *remote_program;
> + char *to_free = 0;
>
> if (data->conn)
> return 0;
>
> + remote_program = (for_push ? data->options.receivepack
> + : data->options.uploadpack);
> +
> + if (transport->smart_options->transport_version >= 2) {
> + to_free = xmalloc(strlen(remote_program) + 12);
> + sprintf(to_free, "%s-%d", remote_program,
> + transport->smart_options->transport_version);
> + remote_program = to_free;
> + }
> +
It looks to me that the caller should pass "upload-pack-2" here in
data->options.uploadpack already. We should not need to manipulate the
uploadpack's program name. Not sure how complicated it would be
though.
> data->conn = git_connect(data->fd, transport->url,
> -for_push ? data->options.receivepack :
> -data->options.uploadpack,
> +remote_program,
> verbose ? CONNECT_VERBOSE : 0);
>
> + free(to_free);
> +
> return 0;
> }
>
> --
> 2.4.1.345.gab207b6.dirty
>
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFCv2 13/16] fetch-pack: use the configured transport protocol
On Tue, Jun 2, 2015 at 7:02 AM, Stefan Beller wrote: > builtin/fetch-pack.c | 22 ++- > fetch-pack.c | 109 > +++ > fetch-pack.h | 1 + > 3 files changed, 80 insertions(+), 52 deletions(-) And the companion changes in transport-helper.c should be in this patch as well to support smart http. I don't think there is any problem with how you store the "version" (or "transport_version", you should probably stick to one name) though. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 00/11] Protocol version 2, again!
On Tue, Jun 2, 2015 at 12:49 AM, Stefan Beller wrote:
> However the client side with builtin/fetch, builtin/fetch-pack, fetch-pack
> is a bit of a mystery to me, as I cannot fully grasp the difference between
> * connect.{h,c}
> * remote.{h.c}
> * transport.{h.c}
> there. All of it seems to be doing network related stuff, but I have trouble
> getting the big picture. I am assuming all of these 3 are rather a low level,
> used like a library, though there must be even more hierarchy in there,
> connect is most low level judging from the header file and used by
> the other two.
> transport.h seems to provide the most toplevel library stuff as it includes
> remote.h in its header?
I think transport.c is there to support non-native protocols (and
later on, smart http). So yeah it's basically the API for fetches and
pushes. git-log over those files may reveal their purposes, especially
the few first versions of them.
> The problem I am currently trying to tackle, is passing the options through
> all
> the layers early on. so in a few places we have code like
>
> switch (version) {
> case 2: /* first talk about capabilities, then get the heads */
> get_remote_capabilities(data->fd[0], NULL, 0);
> select_capabilities();
> request_capabilities(data->fd[1]);
> /* fall through */
> case 1:
> get_remote_heads(data->fd[0], NULL, 0, &refs,
> for_push ? REF_NORMAL : 0,
> &data->extra_have,
> &data->shallow);
> break;
> default:
> die("BUG: Transport version %d not supported", version);
> break;
> }
>
> and the issue I am concerned about is the select_capabilities as well as
> the request_capabilities function here. The select_capabilities functionality
> is currently residing in the high level parts of the code as it both depends
> on
> the advertised server capabilities and on the user input (--use-frotz or
> config
> options), so the capability selection is done in fetchpack.c
>
> So there are 2 routes to go: Either we leave the select_capabilities in the
> upper layers (near the actual high level command, fetch, fetchpack) or we put
> it into the transport layer and just passing in a struct what the user
> desires.
> And when the users desire doesn't meet the server capabilities we die deep
> down
> in the transport layer.
I read the latest re-roll and I think the placement makes sense. You
can't put protocol specific at transport level because "pack protocol"
is just one of the supported protocols. There is smart-http (which
shares a bunch of code, but from transport perspective is a separate
protocol), and then user-defined protocols that know nothing about
this v2.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] index-pack: fix truncation of off_t in comparison
On Thu, Jun 4, 2015 at 7:35 PM, Jeff King wrote:
> Commit c6458e6 (index-pack: kill union delta_base to save
> memory, 2015-04-18) refactored the comparison functions used
> in sorting and binary searching our delta list. The
> resulting code does something like:
>
> int cmp_offsets(off_t a, off_t b)
> {
> return a - b;
> }
>
> This works most of the time, but produces nonsensical
> results when the difference between the two offsets is
> larger than what can be stored in an "int".
Ugh.. thanks for fixing this. Too bad neither gcc, clang or coverity
spotted this.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pack files, standards compliance, and efficiency
On Fri, Jun 5, 2015 at 4:45 PM, Jeff King wrote: > On Fri, Jun 05, 2015 at 01:41:21AM +, brian m. carlson wrote: > >> However, with the object_id conversion, we run into a problem: casting >> those unsigned char * values into struct object_id * values is not >> allowed by the C standard. There are two possible solutions: copying; >> and the just-do-it solution, where we cast and hope for the best. > > I'm not sure if it does violate the standard. The address of the first > element of a struct is guaranteed to match the address of the struct > itself. The object_id.hash member is an array of unsigned char, so there > are no alignment issues. It might run afoul of rules about casting > between pointer types (i.e., pointers for all types are not guaranteed > to be the same size). The standard dictates that "char *" and "void *" > are the same size, and big enough to hold a pointer to anything, so I > think it might even be OK. > > But I'm not even sure that line of thinking is all that interesting. > Even if we are violating some dark corner of the standard, this > definitely falls into the "it's useful and works on all sane machines" > category. We also do much worse things with struct-casting mmap'd data > elsewhere (e.g., see the use of "struct pack_header"). It works fine in > practice as long as you are careful about alignment and padding issues. > > So my vote would be to retain the cast. This is very low-level, > performance-sensitive code. I did some very naive timings and didn't see > any measurable change from your patch, but I also don't think we are > seeing a real portability benefit to moving to the copy, so I'd prefer > to keep the status quo. I'm more concerned about breaking object_id abstraction than C standard. Let's think a bit about future. I suppose we need to support both sha-1 and sha-512, at least at the source code level. That might make casting tricky. Maybe we should deal with it now instead of delaying because if the final solution is vastly different, we may be redoing this conversion again. In any case, if we cast, we should make it grep-able (maybe hide the casting in a macro so we can grep the macro's name) so we can examine them when the time comes for us to move away from sha-1. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Suggestion: make git checkout safer
On Fri, Jun 5, 2015 at 4:32 PM, Ed Avis wrote: > Torsten Bögershausen web.de> writes: > >>Do you think you can write a patch to improve the documentation ? > > Here is my attempt, but it is only a starting point. > > diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt > index d263a56..ee25354 100644 > --- a/Documentation/git-checkout.txt > +++ b/Documentation/git-checkout.txt > @@ -3,7 +3,7 @@ git-checkout(1) > > NAME > > -git-checkout - Checkout a branch or paths to the working tree > +git-checkout - Overwrite working tree files with a given branch Maybe "switch branches or reset working tree files"? -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pack files, standards compliance, and efficiency
On Fri, Jun 5, 2015 at 5:36 PM, Jeff King wrote: > On Fri, Jun 05, 2015 at 05:14:25PM +0700, Duy Nguyen wrote: > >> I'm more concerned about breaking object_id abstraction than C >> standard. Let's think a bit about future. I suppose we need to support >> both sha-1 and sha-512, at least at the source code level. > > I think that's going to be a much bigger issue, because we are casting > out of a defined, on-disk data structure here. So I'd rather defer any > code changes around this until we see what the new data structure (and > the new code) look like. Yeah.. I don't have a clear idea what it would look like either. What I didn't think of is this object_id may turn around and influence how new on-disk format looks like, to make it easier to work with object_id. So there's an option.. >> That might make casting tricky. Maybe we should deal with it now >> instead of delaying because if the final solution is vastly different, >> we may be redoing this conversion again. In any case, if we cast, we >> should make it grep-able (maybe hide the casting in a macro so we can >> grep the macro's name) so we can examine them when the time comes for >> us to move away from sha-1. > > I think that is sensible. Something like: > > #define SHA1_TO_OBJID(sha1) ((struct object_id *)sha1) > > would probably be a good start. Yep. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On undoing a forced push
>From a thread on Hacker News. It seems that if a user does not have
access to the remote's reflog and accidentally forces a push to a ref,
how does he recover it? In order to force push again to revert it
back, he would need to know the remote's old SHA-1. Local reflog does
not help because remote refs are not updated during a push.
This patch prints the latest SHA-1 before the forced push in full. He
then can do
git push +:
He does not even need to have the objects that refers
to. We could simply push an empty pack and the the remote will happily
accept the force, assuming garbage collection has not happened. But
that's another and a little more complex patch.
Is there any other way to undo a forced push?
-- 8< --
diff --git a/transport.c b/transport.c
index f080e93..6bd6a64 100644
--- a/transport.c
+++ b/transport.c
@@ -657,16 +657,17 @@ static void print_ok_ref_status(struct ref *ref, int
porcelain)
"[new branch]"),
ref, ref->peer_ref, NULL, porcelain);
else {
- char quickref[84];
+ char quickref[104];
char type;
const char *msg;
- strcpy(quickref, status_abbrev(ref->old_sha1));
if (ref->forced_update) {
+ strcpy(quickref, sha1_to_hex(ref->old_sha1));
strcat(quickref, "...");
type = '+';
msg = "forced update";
} else {
+ strcpy(quickref, status_abbrev(ref->old_sha1));
strcat(quickref, "..");
type = ' ';
msg = NULL;
-- 8< --
--
To unsubscribe from this list: send the line "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: On undoing a forced push
On Tue, Jun 9, 2015 at 11:29 PM, Johannes Schindelin wrote: > Sorry to chime in so late in the discussion, but I think that the > `--force-with-lease` option is what you are looking for. It allows you to > force-push *but only* if the forced push would overwrite the ref we expect, > i.e. (simplified, but you get the idea) `git push --force-with-lease > ` will *only* succeed if the remote's agrees with the local > `refs/remotes//`. > > If you use `--force-with-lease`, you simply cannot force-forget anything on > the remote side that you cannot undo (because you have everything locally you > need to undo it). Yeah I recall Junio did something about pushes.. I was about to suggest that we promote force-with-lease to default --force and current --force becomes --force --force. But there's this from commit 2233ad4 (Merge branch 'jc/push-cas' - 2013-09-09) that makes me hesitate The logic to choose the default implemented here is fragile (e.g. "git fetch" after seeing a failure will update the remote-tracking branch and will make the next "push" pass, defeating the safety pretty easily). It is suitable only for the simplest workflows, and it may hurt users more than it helps them. Either way I still want to provide an escape hatch for --force as it's good to reduce the number of unrecoverable operations down. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: On undoing a forced push
On Tue, Jun 9, 2015 at 10:00 PM, brian m. carlson
wrote:
> On Tue, Jun 09, 2015 at 07:12:21PM +0700, Duy Nguyen wrote:
>> diff --git a/transport.c b/transport.c
>> index f080e93..6bd6a64 100644
>> --- a/transport.c
>> +++ b/transport.c
>> @@ -657,16 +657,17 @@ static void print_ok_ref_status(struct ref *ref, int
>> porcelain)
>> "[new branch]"),
>> ref, ref->peer_ref, NULL, porcelain);
>> else {
>> - char quickref[84];
>> + char quickref[104];
>
> You've increased this by 20, but you're adding 40 characters to the
> strcpy. Are you sure that's enough?
>
> Also, you might consider writing this in terms of GIT_SHA1_HEXSZ, as it
> will be more obvious that this depends on that value. If you don't now,
> I will later.
It's a demonstration patch and I didn't pay much attention. I think
converting this quickref to strbuf may be better though, when you
convert this file to object_id.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/8] sha1_file: introduce has_object_file helper.
On Tue, Jun 9, 2015 at 11:28 PM, brian m. carlson
wrote:
> diff --git a/sha1_file.c b/sha1_file.c
> index 7e38148..09f7f03 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -3173,6 +3173,11 @@ int has_sha1_file(const unsigned char *sha1)
> return find_pack_entry(sha1, &e);
> }
>
> +int has_object_file(const struct object_id *oid)
> +{
> + return has_sha1_file(oid->hash);
> +}
> +
This version could be "static inline" and placed in cache.h. Though it
may be premature optimization. On top of my head I can't recall any
place where has_sha1_file() is used so many times for this extra call
to become significant overhead.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] index-pack: avoid excessive re-reading of pack directory
On Wed, Jun 10, 2015 at 9:00 PM, Jeff King wrote: > On Tue, Jun 09, 2015 at 08:46:24PM -0700, Shawn Pearce wrote: > >> > This patch introduces a "quick" flag to has_sha1_file which >> > callers can use when they would prefer high performance at >> > the cost of false negatives during repacks. There may be >> > other code paths that can use this, but the index-pack one >> > is the most obviously critical, so we'll start with >> > switching that one. >> >> Hilarious. We did this in JGit back in ... uhm before 2009. :) >> >> But its Java. So of course we had to do optimizations. > > This is actually how Git did it up until v1.8.4.2, in 2013. I changed it > then because the old way was racy (and git could flakily report refs as > broken and skip them during repacks!). > > If you are doing it the "quick" way everywhere in JGit, you may want to > reexamine the possibility for races. :) I was expecting this :D >> > @@ -3169,6 +3169,8 @@ int has_sha1_file(const unsigned char *sha1) >> > return 1; >> > if (has_loose_object(sha1)) >> > return 1; >> > + if (flags & HAS_SHA1_QUICK) >> > + return 0; >> > reprepare_packed_git(); >> > return find_pack_entry(sha1, &e); >> >> Something else we do is readdir() over the loose objects and store >> them in a map in memory. That way we avoid stat() calls during that >> has_loose_object() path. This is apparently a win enough of the time >> that we always do that when receiving a pack over the wire (client or >> server). > > Yeah, I thought about that while writing this. It would be a win as long > as you have a small number of loose objects and were going to make a > large number of requests (otherwise you are traversing even though > nobody is going to look it up). According to perf, though, loose object > lookups are not a large expenditure[1]. > > I'm also hesitant to go that route because it's basically caching, which > introduces new opportunities for race conditions when the cache is stale > (we do the same thing with loose refs, and we have indeed run into races > there). Watchman may help avoid races _with_ caching. But we need to support both ways in that case, falling back to normal file poking when watchman gives up, or when we're on Windows. Extra work needs big enough performance gain to justify. I haven't seen that gain yet. > [1] As measured mostly by __d_lookup_rcu calls. Of course, my patch > gives a 5% improvement over the original, and we were not spending > 5% of the time there originally. I suspect part of the problem is > that we do the lookup under a lock, so the longer we spend there, > the more contention we have between threads, and the less > parallelism. Indeed, I just did a quick repeat of my tests with > pack.threads=1, and the size of the improvement shrinks. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clone: check if server supports shallow clones
On Thu, Jun 11, 2015 at 2:05 AM, Jeff King wrote:
> On Wed, Jun 10, 2015 at 02:35:20PM -0400, Mike Edgar wrote:
>
>> When the user passes --depth to git-clone the server's capabilities are
>> not currently consulted. The client will send shallow requests even if
>> the server does not understand them, and the resulting error may be
>> unhelpful to the user. This change pre-emptively checks so git-clone can
>> exit with a helpful error if necessary.
>
> This sounds like a good thing to do, but...
>
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -944,6 +944,9 @@ int cmd_clone(int argc, const char **argv, const char
>> *prefix)
>>
>> refs = transport_get_remote_refs(transport);
>>
>> + if (option_depth && !is_local && !server_supports("shallow"))
>> + die(_("Server does not support shallow clients"));
>> +
>
> It feels a little weird to be checking the option here in cmd_clone.
> The transport layer knows we have specified a depth, so it seems like a
> more natural place for it (or possibly even lower, in the actual
> git-protocol code).
>
> That being said, I think the current capabilities handling is a bit
> messy and crosses module boundaries freely. So I would not be surprised
> if this is the most reasonable place to put it. But it does make me
> wonder whether "git fetch --depth=..." needs the same treatment.
>
> I see that do_fetch_pack checks server_supports("shallow"). Is that
> enough to cover all fetch cases? And if it is, why does it not cover the
> matching clone cases?
I think this replacement check would do
if ((args->depth > 0 || is_repository_shallow()) && !server_supports("shallow"))
die("Server does not support shallow clients");
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Failed assertion in pathspec.c
On Sun, Jun 14, 2015 at 12:18 AM, Sami Boukortt wrote: > git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len > <= item->len && item->prefix <= item->len' failed. Known issue, but no one stepped up to fix it yet http://thread.gmane.org/gmane.comp.version-control.git/267095 -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Failed assertion in pathspec.c
On Sun, Jun 14, 2015 at 5:21 PM, Sami Boukortt wrote: >> On Sun, Jun 14, 2015 at 12:18 AM, Sami Boukortt >> wrote: >> > git: pathspec.c:317: prefix_pathspec: Assertion >> > `item->nowildcard_len <= item->len && item->prefix <= item->len' >> > failed. >> >> Known issue, but no one stepped up to fix it yet >> >> http://thread.gmane.org/gmane.comp.version-control.git/267095 > > Oh, I see. Sorry for the duplicate report. No, it's actually good. It reminded me about this bug again. Maybe I'll do something about it :) -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pathspec: adjust prefixlen after striping trailing slash
On Thu, Apr 23, 2015 at 10:47 AM, Junio C Hamano wrote: > On Wed, Apr 22, 2015 at 3:32 PM, Jens Lehmann wrote: >> ... >>> But it is unclear if we should still do (2) when "subrepo/.git" is >>> no longer there. That has to be done manually and it may be an >>> indication that is clear enough that the end user wants the >>> directory to be a normal directory without any submodule involved, >>> in which case it may match the expectation of the user better to >>> just nuke the corresponding 16 entry in the index and replace it >>> with files in there. I dunno. >> >> The user having removed subrepo/.git is just one reason for that. >> Another is a user adding a file in an unpopulated work tree of a >> not initialized submodule. I doubt that simply nuking the 16 >> entry would be the right thing to do in this case, I expect this >> to be a pilot error we should barf about ;-) > > OK, that sounds sensible. There are more to this "submodule vs the world". When .git is gone, if you expect to warn instead of deleting 16. You may want the same for the opposite direction: when b/c is in the index and you add b/.git, you may want "git add b" to complain too (right not it ignores b/.git and updates b/c). We may need more surgery around this area. It seems to be the "Path %s is in submodule" check (in pathspec.c) is at the wrong place. We have d/f check in add_index_entry_with_check. That should catch it, at the core, not from call sites like add or update-index. But that check is not active... The first version of "path is in submodule" is in 2ce53f9 (git add: do not add files from a submodule - 2009-01-02). Back then dir.c does not intervene. If you have "b" as 16 in the index and tries to add b/c, the ok_to_replace feature kicks in and kills 'b'. This is why d/f check is turned off. This ok_to_replace is to deal with symlinks, see 192268c (Add git-update-cache --replace option. - 2005-05-07). I think we should stop the ok-to-replace feature when submodules are involved, we consider submdules much more valuable than symlinks. If we do this, I think we can delete those "Path is in submodule" trick becaue the index core can handle it well. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] git-checkout.txt: Document "git checkout " better
On Wed, Jun 17, 2015 at 2:54 PM, Torsten Bögershausen wrote: > -git-checkout - Checkout a branch or paths to the working tree > +git-checkout - Switch branches or restore changes I didn't follow closely the previous discussion. Forgive me if this is already discussed, but I would keep the "in the working tree". "Restore changes" alone seems vague. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] trace: add GIT_TRACE_STDIN
On Wed, Jun 17, 2015 at 4:20 AM, Jeff King wrote: > On Tue, Jun 16, 2015 at 03:49:07PM -0400, Jeff King wrote: > >> Another option would be to stop trying to intercept stdin in git.c, and >> instead make this a feature of run-command.c. That is, right before we >> exec a process, tee its stdin there. That means that you cannot do: >> >> GIT_TRACE_STDIN=/tmp/foo.out git foo >> >> to collect the stdin of foo. But that is not really an interesting case >> anyway. You can run "tee" yourself if you want. The interesting cases >> are the ones where git is spawning a sub-process, and you want to >> intercept the data moving between the git processes. > > Hmm. I guess we do not actually have to move the stdin interception > there. We can just move the config-checking there, like the patch below. > > It basically just converts trace.foo.bar into GIT_TRACE_BAR when we are > running "foo" as a git command. This does work, but is perhaps > potentially confusing to the user, because it only kicks in when _git_ > runs "foo". IOW, this works: > > git config trace.upload-pack.foo /path/to/foo.out > git daemon I wonder if we could do it a bit differently. Instead of GIT_TRACE_STDIN, I would add GIT_TRACE_HOOK that points to a script. Whenever a command is run via run-command interface, the actual command line to be executed would be " ". Because this script is given full command line, it can decide to trace something if the command name is matched (or arguments are matched) or just execute the original command. It's more flexible that trace.* config keys. We also have an opportunity to replace builtin commands, like pack-objects, in command pipeline in fetch or push with something else, to inject errors or whatever. It can be done manually, but it's not easy or convenient. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Fwd: New Defects reported by Coverity Scan for git
I think Coverity caught this correctly.
** CID 1306846: Memory - illegal accesses (USE_AFTER_FREE)
/builtin/pull.c: 287 in config_get_rebase()
*** CID 1306846: Memory - illegal accesses (USE_AFTER_FREE)
/builtin/pull.c: 287 in config_get_rebase()
281
282 if (curr_branch) {
283 char *key = xstrfmt("branch.%s.rebase",
curr_branch->name);
284
285 if (!git_config_get_value(key, &value)) {
286 free(key);
>>> CID 1306846: Memory - illegal accesses (USE_AFTER_FREE)
>>> Passing freed pointer "key" as an argument to "parse_config_rebase".
287 return parse_config_rebase(key, value, 1);
288 }
289
290 free(key);
291 }
292
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] git-checkout.txt: Document "git checkout " better
On Thu, Jun 18, 2015 at 2:58 AM, Junio C Hamano wrote: > Torsten Bögershausen writes: > >> On 2015-06-17 21.23, Junio C Hamano wrote: >> [] Basically, I'm fine with anything starting with "Switch branches or", but please do change the headline ;-). >>> >>> Likewise; I agree "switch branches or" part is good. >> >> How about this: >> >> git-checkout - Switch branches or restore changes to the working tree > > Gahh. We are NOT restoring CHANGES. We are restoring the whole > contents to a path. "the whole contents" is only true when --patch is not used, I think. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] strbuf: stop out-of-boundary warnings from Coverity
On Wed, Jun 17, 2015 at 03:12:35PM -0400, Jeff King wrote:
> On Wed, Jun 17, 2015 at 10:58:10AM -0700, Stefan Beller wrote:
>
> > > Just make strbuf_slopbuf[] large enough to keep Coverity happy. If it's
> > > happy, we'll have cleaner defect list
> >
> > It's down 31 defects, roughly 10% of all things coverity detected as
> > problematic.
> > YAY!
>
> That's a good thing. I do find the solution a little gross, though. I
> wonder if there is a way we can tell coverity more about how strbuf
> works. I've noticed similar problems with string_list, where it
> complains that we are touching list->items, which was assigned to NULL
> (of course it was, but then after that we did string_list_append!).
There's "function modeling" where you write simplified (and likely
incorrect) version of a function to correct how coverity's
understanding of that function. I searched, there's no "data
modeling". I think I have the user manual, but haven't looked through
it yet.
On Thu, Jun 18, 2015 at 2:25 AM, Junio C Hamano wrote:
> I actually think this is too ugly to live.
Well, I have another option below. Let's see how people feel about it.
> If coverity is buggy and unusable, why aren't we raising that issue
> to them?
It's technically correct though. The key point here is strbuf_slopbuf[0]
is NUL, but that cannot be statically determined. And we may also need
to teach it about strcmp' and friends' semantics. That's probably too
much for a static analyzer.
The last resort is simply filter out a whole class of warnings.
Probably good enough if both patches look equally ugly.
-- 8< --
Subject: [PATCH] strbuf: kill strbuf_slopbuf, in favor of ""
A lot of "out-of-bound access" warnings on scan.coverity.com is because
it does not realize this strbuf_slopbuf[] is in fact initialized with a
single and promised to never change. But that promise could be broken if
some caller attempts to write to strbuf->buf[0] write after STRBUF_INIT.
We really can't do much about it. But we can try to put strbuf_slopbuf
in .rodata section, where writes will be caught by the OS with memory
protection support. The only drawback is people can't do
"buf->buf == strbuf_slopbuf" any more. Luckily nobody does that in the
current code base.
---
strbuf.c | 19 ++-
strbuf.h | 11 ---
2 files changed, 14 insertions(+), 16 deletions(-)
diff --git a/strbuf.c b/strbuf.c
index 0d4f4e5..9f91229 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -11,17 +11,10 @@ int starts_with(const char *str, const char *prefix)
return 0;
}
-/*
- * Used as the default ->buf value, so that people can always assume
- * buf is non NULL and ->buf is NUL terminated even for a freshly
- * initialized strbuf.
- */
-char strbuf_slopbuf[1];
-
void strbuf_init(struct strbuf *sb, size_t hint)
{
sb->alloc = sb->len = 0;
- sb->buf = strbuf_slopbuf;
+ sb->buf = (char*)"";
if (hint)
strbuf_grow(sb, hint);
}
@@ -52,7 +45,7 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len,
size_t alloc)
sb->len = len;
sb->alloc = alloc;
strbuf_grow(sb, 0);
- sb->buf[sb->len] = '\0';
+ strbuf_terminate(sb);
}
void strbuf_grow(struct strbuf *sb, size_t extra)
@@ -77,7 +70,7 @@ void strbuf_rtrim(struct strbuf *sb)
{
while (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1]))
sb->len--;
- sb->buf[sb->len] = '\0';
+ strbuf_terminate(sb);
}
void strbuf_ltrim(struct strbuf *sb)
@@ -88,7 +81,7 @@ void strbuf_ltrim(struct strbuf *sb)
sb->len--;
}
memmove(sb->buf, b, sb->len);
- sb->buf[sb->len] = '\0';
+ strbuf_terminate(sb);
}
int strbuf_reencode(struct strbuf *sb, const char *from, const char *to)
@@ -380,7 +373,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
strbuf_grow(sb, 8192);
}
- sb->buf[sb->len] = '\0';
+ strbuf_terminate(sb);
return sb->len - oldlen;
}
@@ -496,7 +489,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int
term)
if (ch == EOF && sb->len == 0)
return EOF;
- sb->buf[sb->len] = '\0';
+ strbuf_terminate(sb);
return 0;
}
#endif
diff --git a/strbuf.h b/strbuf.h
index 01c5c63..d8346ee 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -67,8 +67,13 @@ struct strbuf {
char *buf;
};
-extern char strbuf_slopbuf[];
-#define STRBUF_INIT { 0, 0, strbuf_slopbuf }
+#define STRBUF_INIT { 0, 0, (char*)"" }
+
+static inline void strbuf_terminate(struct strbuf *sb)
+{
+ if (sb->buf[sb->len])
+ sb->buf[sb->len] = '\0';
+}
/**
* Life Cycle Functions
@@ -149,7 +154,7 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t
len)
if (len > (sb->alloc ? sb->alloc - 1 : 0))
die("BUG: strbuf_setlen() beyond buffer");
sb->len = len;
- sb->buf[len] = '\0';
+ strbuf_terminate(sb)
Re: [PATCH 3/3] trace: add GIT_TRACE_STDIN
On Thu, Jun 18, 2015 at 2:10 AM, Jeff King wrote:
> On Wed, Jun 17, 2015 at 05:04:04PM +0700, Duy Nguyen wrote:
>
>> I wonder if we could do it a bit differently. Instead of
>> GIT_TRACE_STDIN, I would add GIT_TRACE_HOOK that points to a script.
>> Whenever a command is run via run-command interface, the actual
>> command line to be executed would be "
>> ".
>
> Hmm, yeah, I like that. It's even more flexible, and it is much more
> obvious why it works only for run-command. If we feed the resulting
> "hooked" command to the shell, I think you could do:
>
> GIT_TRACE_HOOK='
> f() {
> case "$1 $2" in
> git pack-objects)
> tee /tmp/foo.out | "$@"
> ;;
> esac
> }; f
> '
>
> That is not 100% correct (you would miss "git --some-arg pack-objects"),
Yeah, flexibility always comes with traps and pitfalls.
> but it is probably fine in practice for debugging sessions. It is a bit
> more complicated to use, but I really like the flexibility (I can
> imagine that "GIT_TRACE_HOOK=gdbserver localhost:1234" would come in
> handy).
>
>> Because this script is given full command line, it can decide to trace
>> something if the command name is matched (or arguments are matched) or
>> just execute the original command. It's more flexible that trace.*
>> config keys. We also have an opportunity to replace builtin commands,
>> like pack-objects, in command pipeline in fetch or push with something
>> else, to inject errors or whatever. It can be done manually, but it's
>> not easy or convenient.
>
> My other motive for trace.* was that we could have something like
> "trace.prune", and have git-prune provide verbose debugging information.
> We have custom patches like that on GitHub servers, which we've used to
> debug occasional weirdness (e.g., you find that an object is missing
> from a repo, but you have no clue why it went away; was it never there,
> did somebody prune it, did it get dropped from a pack?).
>
> I can send those upstream, but it would be nice not to introduce a
> totally separate tracing facility when trace_* is so close. But it
> needs:
>
> 1. To be enabled by config, not environment.
>
> 2. To support some basic output filename flexibility so the output can
> be organized (we write the equivalent of GIT_TRACE_FOO to
> $GIT_DIR/ghlog_foo/-MM-DD/-MM-DDTHH:MM:SS.PID).
>
> For (1), we could just load trace.* in git_default_config; you couldn't
> use it with any "early" tracing that happens before then, but I think in
> practice it would be fine for most traces.
>
> For (2), I think we could accomplish that with %-placeholders (like my
> earlier patch), and the ability to write relative paths into $GIT_DIR
> (again, you couldn't do this for "early" traces, but you could for other
> stuff).
>
> Or we could just do nothing. I'm not sure if anybody else is actually
> interested in verbose-logging patches like these.
I'm not stopping you from doing this, just to be clear. I was just
trying to convince you to do something extra that I wanted to use ;)
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] strbuf: stop out-of-boundary warnings from Coverity
On Thu, Jun 18, 2015 at 09:46:09AM -0700, Junio C Hamano wrote:
> Duy Nguyen writes:
>
> > The last resort is simply filter out a whole class of warnings.
> > Probably good enough if both patches look equally ugly.
> >
> > -- 8< --
> > Subject: [PATCH] strbuf: kill strbuf_slopbuf, in favor of ""
> >
> > A lot of "out-of-bound access" warnings on scan.coverity.com is because
> > it does not realize this strbuf_slopbuf[] is in fact initialized with a
> > single and promised to never change. But that promise could be broken if
> > some caller attempts to write to strbuf->buf[0] write after STRBUF_INIT.
> >
> > We really can't do much about it. But we can try to put strbuf_slopbuf
> > in .rodata section, where writes will be caught by the OS with memory
> > protection support. The only drawback is people can't do
> > "buf->buf == strbuf_slopbuf" any more. Luckily nobody does that in the
> > current code base.
> > ---
>
> Hmph, would declaring slopbuf as "const char [1]" (and sprinkling
> the "(char *)" cast) have the same effect, I wonder?
I remember I tried that and failed with a compile error. I just tried
again, this time it worked. Must have made some stupid mistake in my
first try.
Anyway it does not put strbuf_slopbuf in .rodata. "./git" does not
segfault with this patch
-- 8< --
diff --git a/git.c b/git.c
index 44374b1..960e375 100644
--- a/git.c
+++ b/git.c
@@ -621,7 +621,11 @@ int main(int argc, char **av)
const char **argv = (const char **) av;
const char *cmd;
int done_help = 0;
+ struct strbuf sb = STRBUF_INIT;
+ sb.buf[0] = 'Z';
+ printf("%c\n", strbuf_slopbuf[0]);
+ return 0;
startup_info = &git_startup_info;
cmd = git_extract_argv0_path(argv[0]);
diff --git a/strbuf.c b/strbuf.c
index 0d4f4e5..3a0d4c9 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -16,12 +16,12 @@ int starts_with(const char *str, const char *prefix)
* buf is non NULL and ->buf is NUL terminated even for a freshly
* initialized strbuf.
*/
-char strbuf_slopbuf[1];
+const char strbuf_slopbuf[1];
void strbuf_init(struct strbuf *sb, size_t hint)
{
sb->alloc = sb->len = 0;
- sb->buf = strbuf_slopbuf;
+ sb->buf = (char *)strbuf_slopbuf;
if (hint)
strbuf_grow(sb, hint);
}
diff --git a/strbuf.h b/strbuf.h
index 01c5c63..eab7307 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -67,8 +67,8 @@ struct strbuf {
char *buf;
};
-extern char strbuf_slopbuf[];
-#define STRBUF_INIT { 0, 0, strbuf_slopbuf }
+extern const char strbuf_slopbuf[];
+#define STRBUF_INIT { 0, 0, (char *)strbuf_slopbuf }
/**
* Life Cycle Functions
-- 8< --
> > +static inline void strbuf_terminate(struct strbuf *sb)
> > +{
> > + if (sb->buf[sb->len])
> > + sb->buf[sb->len] = '\0';
> > +}
>
> This is so that you can call things like strbuf_rtrim() immediately
> after running strbuf_init() safely, but I think it needs a comment
> to save people from wondering what is going on, e.g. "this is not an
> optimization to avoid assigning NUL to a place that is already NUL;
> a freshly initialized strbuf points at an unwritable piece of NUL
> and we do not want to cause a SEGV".
Sure, if we go with this direction.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] strbuf: stop out-of-boundary warnings from Coverity
On Fri, Jun 19, 2015 at 5:50 PM, Remi Galan Alfonso
wrote:
> Duy Nguyen writes:
>
>> + sb.buf[0] = 'Z';
>> + printf("%c\n", strbuf_slopbuf[0]);
>> + return 0;
>> startup_info = &git_startup_info;
>
> I might be wrong, but I definitely think that this
> printf and return 0 are some debug lines that you
> forgot to remove.
Yes it's debug code. I hoped that if I made a mistake forcing the
segfault, people would notice.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
Re: [PATCH] Add list-all-objects command
On Mon, Jun 22, 2015 at 2:20 AM, Charles Bailey wrote: > From: Charles Bailey > > list-all-objects is a command to print the ids of all objects in the > object database of a repository. It is designed as a low overhead > interface for scripts that want to analyse all objects but don't require > the ordering implied by a revision walk. > > It will list all objects, loose and packed, and will include unreachable > objects. Nit picking, but perhaps we should allow to select object source: loose, packed, alternates.. These info are available now and cheap to get. It's ok not to do it now though. Personally I would name this command "find-objects" (after unix command "find") where we could still filter objects _not_ based on object content. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in
Re: apply --cached --whitespace=fix now failing on items added with "add -N"
On Mon, Jun 22, 2015 at 9:29 PM, Patrick Higgins wrote:
> I like to use git to remove trailing whitespace from my files. I use
> the following ~/.gitconfig to make this convenient:
>
> [alias]
> wsadd = "!sh -c 'git diff -- \"$@\" | git apply --cached
> --whitespace=fix;\
> git checkout -- ${1-.} \"$@\"' -"
>
> The wsadd alias doesn't work with new files, so I have to use "git add
> -N" on them first. As of a week or two ago, the "apply --cached" step
> now fails with the following, assuming a new file named bar containing
> "foo " has been added with "add -N":
>
> $ git diff -- "$@" | git apply --cached --whitespace=fix
> :7: trailing whitespace.
> foo
> error: bar: already exists in index
>
> The final "git checkout" at the end of wsadd truncates my file. I've
> changed the ";" to a "&&" to fix the truncation.
>
> Were there any recent changes to "git apply" that might have caused this?
Probably fallout from this one, merged to 'master' about 5 weeks ago.
I'll have a look at it tomorrow unless somebody does it first
d95d728 (diff-lib.c: adjust position of i-t-a entries in diff - 2015-03-16)
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
Re: RFC/Pull Request: Refs db backend
On Tue, Jun 23, 2015 at 12:36 PM, Junio C Hamano wrote: > "If there is interest"? Shut up and take my money ;-) Yeah. This may be the next big thing since pack bitmap. It's even better if it enters 'master' hand in hand with pack protocol v2, but I think v2 needs more time. On Tue, Jun 23, 2015 at 7:50 AM, David Turner wrote: > To test this backend's correctness, I hacked test-lib.sh and > test-lib-functions.sh to run all tests under the refs backend. Now we have two. split-index also benefits from running through full test suite like this. I propose we make "make test" run the test suite twice. The first run is with default configuration, no split index, no fancy ref backend. The second run enables split-index and switches to new backend, running through all test cases. In future we can also enable packv4 in this second run. There won't be a third run. When the second ref backend comes, we can switch between the two backends using a random number generator where we control both algorithm and seed, so that when a test fails, the user can give us their seed and we can re-run with the same configuration. > Dozens of tests use manual ref/reflog reading/writing, or create submodules > without passing --refs-backend-type to git init. If those tests are > changed to use the update-ref machinery or test-refs-be-db (or, in the > case of packed-refs, corrupt refs, and dumb fetch tests, are skipped), > the only remaining failing tests are the git-new-workdir tests and the > gitweb tests. I haven't read the series, but I guess you should also add a few tests to run on the first run, so new code is exercised a bit even if people skip the second run. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] introduce "preciousObjects" repository extension
On Tue, Jun 23, 2015 at 5:54 PM, Jeff King wrote:
> diff --git a/builtin/prune.c b/builtin/prune.c
> index 0c73246..fc0c8e8 100644
> --- a/builtin/prune.c
> +++ b/builtin/prune.c
> @@ -218,6 +218,9 @@ int cmd_prune(int argc, const char **argv, const char
> *prefix)
> return 0;
> }
>
> + if (repository_format_precious_objects)
> + die("cannot prune in a precious-objects repo");
> +
> while (argc--) {
> unsigned char sha1[20];
> const char *name = *argv++;
> diff --git a/builtin/repack.c b/builtin/repack.c
> index af7340c..8ae7fe5 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -193,6 +193,9 @@ int cmd_repack(int argc, const char **argv, const char
> *prefix)
> argc = parse_options(argc, argv, prefix, builtin_repack_options,
> git_repack_usage, 0);
>
> + if (delete_redundant && repository_format_precious_objects)
> + die("cannot repack in a precious-objects repo");
> +
> if (pack_kept_objects < 0)
> pack_kept_objects = write_bitmaps;
>
I know both commands have translatable strings that are not marked
with _(). But if you reroll, maybe you could add _() for these new
strings. It's even better if you mark all others in these commands
too, if you have time.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RFC/Pull Request: Refs db backend
On Tue, Jun 23, 2015 at 6:47 PM, Jeff King wrote: > I was thinking of actually moving to a log-structured ref storage. > Something like: > > - any ref write puts a line at the end of a single logfile that > contains the ref name, along with the normal reflog data > > - the logfile is the source of truth for the ref state. If you want to > know the value of any ref, you can read it backwards to find the > last entry for the ref. Everything else is an optimization. > > Let's call the number of refs N, and the number of ref updates in > the log U. > > - we keep a key/value index mapping the name of any branch that exists > to the byte offset of its entry in the logfile. This would probably One key/value mapping per branch, pointing to the latest reflog entry, or one key/valye mapping for each reflog entry? > be in some binary key/value store (like LMDB). Without this, > resolving a ref is O(U), which is horrible. With it, it should be > O(1) or O(lg N), depending on the index data structure. I'm thinking of the user with small or medium repos, in terms of refs, who does not want an extra dependency. If we store one mapping per branch, then the size of this mapping is small enough that the index in a text file is ok. If we also store the offset to the previous reflog entry of the same branch in the current reflog entry, like a back pointer, then we could jump back faster. Or do you have something else in mind? Current reflog structure won't work because I think you bring back the reflog graveyard with this, and I don't want to lose that > - the index can also contain other optimizations. E.g., rather than > point to the entry in the logfile, it can include the sha1 directly > (to avoid an extra level of indirection). It may want to include the > "peeled" value, as the current packed-refs file does. > > - Reading all of the reflogs (e.g., for repacking) is O(U), just like > it is today. Except the storage for the logfile is a lot more > compact than what we store today, with one reflog per file. > > - Reading a single reflog is _also_ O(U), which is not as good as > today. But if each log entry contains a byte offset of the previous > entry, you can follow the chain (it is still slightly worse, because > you are jumping all over the file, rather than reading a compact set > of lines). > > - Pruning the reflog entries from the logfile requires rewriting the > whole thing. That's similar to today, where we rewrite each of the > reflog files. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Enable core.fsyncObjectFiles by default
On Wed, Jun 24, 2015 at 4:57 AM, Stefan Beller wrote: > Linus Torvalds started a discussion[1] if we want to play rather safe > than use defaults which make sense only for the most power users of Git: > >> So git is "safe" in the sense that you won't really lose any data, >> but you may well be inconvenienced. The "fsync each object" config >> option is there in case you don't want that inconvenience, but it >> should be noted that it can make for a hell of a performance impact. > >> Of course, it might well be the case that the actual default >> might be worth turning around. Most git users probably don't >> care about that kind of "apply two hundred patches from Andrew >> Morton" kind of workload, although "rebase a big patch-series" >> does end up doing basically the same thing, and might be more >> common. > > This patch enables fsync_object_files by default. Will this make nfs performance a lot worse or still within acceptable range? -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] apply: fix adding new files on i-t-a entries
On Tue, Jun 23, 2015 at 11:50 PM, Junio C Hamano wrote:
>> This patch tightens the "exists in index" check, ignoring i-t-a
>> entries. For fixing the above problem, only the change in
>> check_to_create() is needed.
>
> And the first thing I noticed and found a bit disturbing was that
> this change (which I think is correct, and happens to match what I
> sent out earlier) was the only thing necessary to make your new test
> pass. IOW, the other changes in this patch have no test coverage.
Because to be honest I don't understand these code well enough to know
how to trigger it :)
>> For other changes,
>>
>> - load_current(), reporting "not exists in index" is better than "does
>>not match index"
>
> Is that error reporting the only side effect from this change?
The only thing that I can see. If an i-t-a entry is returned, it can't
get past verify_index_match because ce_match_stat(). Hmm.. no the
on-disk version is gone, we'll get to checkout_target() where it will
"restore" the entry with an empty file. This is related to checkout
that I will continue later below.
>> - get_current_sha1(), or actually build_fake_ancestor(), we should not
>>add i-t-a entries to the temporary index, at least not without also
>>adding i-t-a flag back
>
> This is part of "am" three-way fallback codepath. I do not think
> the merge-recursive three-way merge code knows and cares about, is
> capable of handling, or would even want to deal with i-t-a entries
> in the first place, so adding an entry as i-t-a bit would not help.
> What the ultimate caller wants from us in this codepath is a tree
> object, and that is written out from the temporary index---and that
> codepath ignores i-t-a entries, so it is correct to omit them from
> the temporary index in the first place. Unlike the previous two
> changes, I think this change deserves a new test.
Will do, after I study some more about this apply.c.
>
>> I think I'm opening a can of worms with d95d728. There's nothing
>> wrong with that patch per se, but with this issue popping up, I need
>> to go over all {cache,index}_name_pos call sites and see what would be
>> the sensible behavior when i-t-a entries are involved.
>
> Yeah, I agree that d95d728 should have been a part of a larger
> series that changes the world order, instead of a single change that
> brings inconsistency to the system.
>
> I cannot offhand convince myself that "apply" is the only casualty;
> assuming it is, I think a reasonable way forward is to keep d95d728
> and adjust "apply" to the new world order. Otherwise, i.e. if there
> are wider fallouts from d95d728, we may instead want to temporarily
> revert it off from 'master', deal with fallouts to "apply" and other
> things, before resurrecting it.
>
> Anything that internally uses "diff-index" is suspect, I think.
Yeah that's one or two more grep runs and more reading.
> What do others think? You seem to ...
>
>> So far blame, rm and checkout-entry and "checkout " are on my
>> to-think-or-fix list. But this patch can get in early to fix a real
>> regression instead of waiting for one big series. A lot more
>> discussions will be had before that series gets in good shape.
>
> ... think that the damage could be quite extensive, so I am inclined
> to say that we first revert d95d728 before going forward.
I'm not opposed to reverting if you think it's the safest option and I
will report back soon after grepping diff-index. But those I mentioned
above have more to do with the fact that an i-t-a entry does exist in
the index in a normal way, so reverting does not help.
Take checkout for example, when you do "git checkout -- foo" where foo
is i-t-a, the file foo on disk will be emptied because the SHA-1 in
the i-t-a entry is an empty blob, mostly to help "git diff". I think
it should behave as if foo is not i-t-a: checkout should error out
about not matching pathspec, or at least not destroy "foo" on disk. To
me, when "ce" is an i-t-a entry, only i-t-a flag and ce_name are
valid, the rest of "ce" should never be accessed.
blame.c's situation is close to check_preimage() where it may read
zero from ce_mode. It may be ok for check_preimage() to take zero as
mode, but I think this is like fixed size buffer vs strbuf again. It
works now, but if the code is reorganized or refactored, then it may
or may not work. Better be safe than sorry and avoid reading something
we should not read in the first place.
>> builtin/apply.c | 8
>> cache.h | 2 ++
>> read-cache.c | 12
>> t/t2203-add-intent.sh | 10 ++
>> 4 files changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/builtin/apply.c b/builtin/apply.c
>> index 146be97..4f813ac 100644
>> --- a/builtin/apply.c
>> +++ b/builtin/apply.c
>> @@ -3344,7 +3344,7 @@ static int load_current(struct image *image, struct
>> patch *patch)
>> if (!patch->is_new)
>> die("BUG: patch to %s is not a creation", patch->o
Re: RFC/Pull Request: Refs db backend
On Wed, Jun 24, 2015 at 1:09 PM, Shawn Pearce wrote: >>> I chose to use LMDB for the database. LMDB has a few features that make >>> it suitable for usage in git: >> >> One of the complaints that Shawn had about sqlite is that there is no >> native Java implementation, which makes it hard for JGit to ship a >> compatible backend. I suspect the same is true for LMDB, but it is >> probably a lot simpler than sqlite (so reimplementation might be >> possible). > > Yes. Whatever the default standard format is for git-core, we need > that format to be easily supportable from JGit. Loading code via JNI > is not "easily supportable". I'm under the impression that this will be opt-in, not completely replacing fs-based ref backend. Anyway, any recommendation about database format or engine that is more friendly to Java and JGit (and preferably has good C support too)? -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] introduce "preciousObjects" repository extension
On Wed, Jun 24, 2015 at 3:12 PM, Jeff King wrote:
> On Tue, Jun 23, 2015 at 06:54:11AM -0400, Jeff King wrote:
>
>> diff --git a/builtin/prune.c b/builtin/prune.c
>> index 0c73246..fc0c8e8 100644
>> --- a/builtin/prune.c
>> +++ b/builtin/prune.c
>> @@ -218,6 +218,9 @@ int cmd_prune(int argc, const char **argv, const char
>> *prefix)
>> return 0;
>> }
>>
>> + if (repository_format_precious_objects)
>> + die("cannot prune in a precious-objects repo");
>> +
>
> By the way, I originally wrote this patch on an older version of git,
> and was surprised that `git gc` broke when I brought it forward. The
> problem was that gc now runs `git prune --worktree`, and my die() was
> affecting that case.
>
> It really seems misplaced to me to make worktree-pruning part of
> git-prune. I imagine the rationale was "well, we are pruning things, so
> let's add an option to this command rather than make a new one". But I
> do not think that follows our usual pattern with gc, which is:
>
> 1. Individual areas of code handle their own cleanup. E.g., "reflog
> expire", "rerere gc".
>
> 2. We tie them together with "git gc", not with "git prune".
>
> So it seems weird that "git prune --worktree" is a fundamentally
> different command than "git prune". I think by (1), it should be a
> separate "git prune-worktree" (or better yet, "git worktree prune", as
> the start of a command for manipulating the list of multiple-worktrees).
>
> Not a _huge_ deal, but if we want to change it, it would be nice to do so
> now before it is part of a release. Thoughts?
I was misled by the generic name "prune" :) (OK my bad, the document
clearly says it's about object database). Maybe we should make an
alias prune-objects.. And you caught me too late, I also added
prune_shallow() in there.
Multiple worktree feature is not released yet so I still have some
time to move it out. Yeah "git worktree prune" makes sense. I think I
need a way to list worktrees anyway. I didn't find good enough reason
to create "git worktree" back then just for listing..
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Cannot clone a linked working directory.
On Wed, Jun 24, 2015 at 5:38 PM, Bjørnar Snoksrud wrote: > Summary: > When creating a linked working directory with `git checkout --to`, you > cannot clone from the local path. This works when cloning the main > repository directory. > > I couldn't find anything the the documentation for `git checkout` that > indicates that this shouldn't work. I didn't think of this use case. If something works on the main worktree then it should also work on linked checkouts. I think I see the problem and will try to fix it in probably a few days (the "git add -N" problem takes higher priority). -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] apply: fix adding new files on i-t-a entries
On Thu, Jun 25, 2015 at 12:05 AM, Junio C Hamano wrote: > Internal "diff-index --cached" is used for another reason you did > not mention (and scripted Porcelains literally use that externally > for the same reason). When we start a mergy operation, we say it is > OK if the working tree has local changes relative to the index, but > we require the index does not have any modifications since the HEAD > was read. > > Side note: some codepaths insist that "diff-index --cached" > and "diff-files" are both clean, so d95d728a is harmless; > the former may say "clean" on i-t-a entries more than > before, but the latter will still catch the difference > between the index and the working tree and stop the caller > from going forward. > > With d95d728a (diff-lib.c: adjust position of i-t-a entries in diff, > 2015-03-16)'s world view, an empty output from "diff-index --cached" > no longer means that. Entries added with any "git add -N" are not > reported, so we would go ahead to record the merge result on top of > that "half-dirty" index. > > Side note: a merge based on unpack-trees has an extra layer > of safety that unpack_trees() does not ignore i-t-a entry as > "not exist (yet)" and notices that the path does exist in > the index but not in HEAD. But that just shows that some > parts of the world do need to consider that having an i-t-a > in the index makes it different from HEAD. > > If the mergy operation goes without any conflict, the next thing we > do typically is to write the index out as a tree (to record in a new > commit, etc.) and we are OK only in that particular case, because > i-t-a entries are ignored. But what would happen when the mergy > operation conflicts? I haven't thought about that fully, but I > doubt it is a safe thing to do in general. > > But that is just one example that there are also other codepaths > that do not want to be fooled into thinking that i-t-a entries do > not exist in the index at all. I think it's clear that you need to revert that commit. I didn't see this at all when I made the commit. > All we learned from the above discussion is that unconditionally > declaring that adding i-t-a entries to the index without doing > anything else should keep the index compare the same to HEAD. > > If d95d728a were only about what wt_status.c sees (and gets reported > in "git status" output), which was what the change wanted to address > anyway, we didn't have to have this discussion. Without realizing > that two kinds of callers want different view out of "diff-index > --cached" and "diff-files", we broke half the world by changing the > world order unconditionally for everybody, I am afraid. > > Perhaps a good and safe way forward to resurrect what d95d728a > wanted to do is to first add an option to tell run_diff_index() and > run_diff_files() which behaviour the caller wants to see, add that > only to the caller in wt-status.c? Then incrementally pass that > option from more callsites that we are absolutely certain that want > this different worldview with respect to i-t-a? Agreed. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR
On Fri, Jun 26, 2015 at 6:56 PM, Jeff King wrote:
> On Fri, Jun 26, 2015 at 05:37:35PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> This is where the "fun" is. The legacy behavior is, if $GIT_WORK_TREE is
>> not set but $GIT_DIR is, cwd is chosen as worktree's top. If you happen
>> to stand at worktree's top when you do this, all is well. If you are in
>> a subdir "foo/bar" (real worktree's top is "foo"), this behavior bites
>> you: your detected worktree is now "foo/bar", but the first run
>> correctly detected worktree as "foo". You get "internal error: work tree
>> has already been set" as a result.
>
> I think this makes sense. I feel like we've dealt with this before, but
> the two previous rounds I found were basically:
>
> - we have GIT_IMPLICIT_WORK_TREE, but that is for the _opposite_ case.
> I.e., when we do not have a work tree and must communicate so to
> later code (including sub-processes).
>
> - a discussion about switching the "work tree defaults to '.' when
> $GIT_DIR is set" behavior yielded almost the identical patch:
>
> http://article.gmane.org/gmane.comp.version-control.git/219196
>
> but we were so wrapped up in the greater discussion we did not apply
> that simple fix. :)
There's also the patch "[PATCH v2] setup.c: set workdir when gitdir is
not default" from Max Kirillov last year (gmane down, no link), so we
have one patch about this every year since 2013 :)
Junio if you are worried about unnecessary setenv, I think Max's
approach is safer as he solves a specific use case. If this problem
pops up again in another use case, we can deal with that again.
>> + if (setenv(GIT_WORK_TREE_ENVIRONMENT, work_tree, 1))
>> + error("Could not set GIT_WORK_TREE to '%s'", work_tree);
>
> Should this be die()? setenv() should basically never fail, but if it
> does, it could be confusing and/or dangerous to run without the variable
> set.
It's a straight copy from set_git_dir() but I guess the situation is a
bit different. If setenv fails in gitdir, no repo is found but if it
fails here we may select a wrong worktree and could wipe it out by
mistake. Will fix if Junio chooses this patch instead of Max's.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 1/9] ref-filter: add %(refname:lalignX) option
On Thu, Jun 25, 2015 at 6:43 PM, Karthik Nayak wrote:
> Add support for %(refname:lalignX) where X is a number.
> This will print a shortened refname aligned to the left
> followed by spaces for a total length of X characters.
> If X is less than the shortened refname size, the entire
> shortened refname is printed.
>
> Mentored-by: Christian Couder
> Mentored-by: Matthieu Moy
> Signed-off-by: Karthik Nayak
> ---
> ref-filter.c | 17 -
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 00d06bf..299b048 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -695,7 +695,22 @@ static void populate_value(struct ref_array_item *ref)
> int num_ours, num_theirs;
>
> formatp++;
> - if (!strcmp(formatp, "short"))
> + if (starts_with(formatp, "lalign")) {
> + const char *valp;
> + int val;
> +
> + skip_prefix(formatp, "lalign", &valp);
> + val = atoi(valp);
> + refname = shorten_unambiguous_ref(refname,
> +
> warn_ambiguous_refs);
> + if (val > strlen(refname)) {
> + struct strbuf buf = STRBUF_INIT;
> + strbuf_addstr(&buf, refname);
> + strbuf_addchars(&buf, ' ', val -
> strlen(refname));
We don't forbid non-ascii characters in refname so you probably want
to use utf8_width() here instead of strlen()
> + free((char *)refname);
> + refname = strbuf_detach(&buf, NULL);
> + }
> + } else if (!strcmp(formatp, "short"))
> refname = shorten_unambiguous_ref(refname,
> warn_ambiguous_refs);
> else if (!strcmp(formatp, "track") &&
> --
> 2.4.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] worktree: replace "checkout --to" with "worktree new"
On Tue, Jun 30, 2015 at 11:56 AM, Eric Sunshine wrote:
> The command "git checkout --to " is something of an anachronism,
> encompassing functionality somewhere between "checkout" and "clone".
> The introduction of the git-worktree command, however, provides a proper
> and intuitive place to house such functionality. Consequently,
> re-implement "git checkout --to" as "git worktree new".
>
> As a side-effect, linked worktree creation becomes much more
> discoverable with its own dedicated command, whereas `--to` was easily
> overlooked amid the plethora of options recognized by git-checkout.
>
> Signed-off-by: Eric Sunshine
> ---
>
> I've long felt that Duy's linked-worktree functionality was a bit oddly
> named as "git checkout --to", but, since I could never come up with a
> better name, I never made mention of it. However, with Duy's
> introduction of the git-worktree command[1], we now have a much more
> appropriate and discoverable place to house the "git checkout --to"
> functionality, and upon seeing his patch, I was ready to reply with the
> suggestion to relocate "git checkout --to" to "git worktree new",
> however, Junio beat me to it[2].
Didn't know you guys were so eager to move this code around :D Jokes
aside, it's good that it's raised now before --to is set in stone.
I think this is like "git checkout -b" vs "git branch". We pack so
many things in 'checkout' that it's a source of both convenience and
confusion. I never use "git branch" to create a new branch and if I
had a way to tell checkout to "move away and delete previous branch",
I would probably stop using "git branch -d/-D" too. "--to" is another
"-b" in this sense.
"git worktree new" definitely makes sense (maybe stick with verbs like
"create", I'm not sure if we have some convention in existing
commands), but should we remove "git checkout --to"? I could do "git
co -b foo --to bar" for example. Maybe "--to" is not used that often
that "git worktree new" would feel less convenient as a replacement.
If we are not sure about "--to" (I'm not), I think we just remove it
now because we can always add it back later.
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 41103e5..8f13281 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -9,16 +9,85 @@ git-worktree - Manage multiple worktrees
> SYNOPSIS
>
> [verse]
> +'git worktree new' [-f] []
Should we follow clone syntax and put the (as destination)
after ("source")? Maybe not, because in the clone case,
explicit destination is optional, not like this.. Or.. maybe
could be optional in this case. 'git worktree new' without a branch
will create a new branch, named closely after the destination.
Existing branch can be specified via an option..
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Avoid the need of "--" when wildcard pathspec is used
On Wed, Jul 1, 2015 at 1:10 AM, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > >> When "--" is lacking from the command line and a command can take both >> revs and paths, the idea is if an argument can be seen as both an >> extended SHA-1 and a path, then "--" is required or git refuses to >> continue. It's currently implemented as: >> ... > > Hmph, how does this relate to 28fcc0b7 (pathspec: avoid the need of > "--" when wildcard is used, 2015-05-02)? A follow-up? "Oops, I did > it wrong"? something else? Arghhh! I vaguely recalled I sent something but I couldn't find it and.. > >> diff --git a/setup.c b/setup.c >> index 82c0cc2..f7cb93b 100644 >> --- a/setup.c >> +++ b/setup.c >> @@ -143,6 +143,8 @@ int check_filename(const char *prefix, const char *arg) >> name = arg + 2; >> } else if (!no_wildcard(arg)) >> return 1; .. if I looked at the context lines, I should have noticed the change was already here! >> + else if (!no_wildcard(arg)) >> + return 1; Seems strange (or expected?) that git cherry-pick just adds this chunk on top anyway.. > Puzzling. You already checked if arg has an wildcard and returned > with 1 if there is none. The added check looks like a no-op to me. Yeah sorry for the noise. The only value this patch adds is tests (and maybe better commit message, the last one still mentions magic pathspec even though it's not about it). I think we can just drop this. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] worktree: new place for "git prune --worktrees"
On Mon, Jun 29, 2015 at 11:13 PM, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy writes:
>
>> Commit 23af91d (prune: strategies for linked checkouts - 2014-11-30)
>> adds "--worktrees" to "git prune" without realizing that "git prune" is
>> for object database only. This patch moves the same functionality to a
>> new command "git worktree".
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy
>> ---
>> In future I probably move the big block of text in git-checkout.txt to
>> git-worktree.txt and add "git worktree list". But let's start with
>> something small and simple before "git prune --worktrees" is shipped
>> out.
>
> Thanks. I notice that after applying this, builtin/prune.c does not
> revert to the original before 23af91d. It stops including "dir.h"
> and it adds an extra blank line. We've been including a header that
> is not necessary even in v2.4.0, it seems. We used to walk
> $GIT_DIR/objects ourselves to find loose object files and "dir.h"
> was needed for is_dot_or_dotdot(); for_each_loose_file_in_objdir()
> is what we use these days to hide the implementation details these
> days; 27e1e22 forgot to remove the inclusion when it did this.
>
> The C code part is mostly just \C-x \C-v and I found nothing
> questionable.
I was about to prepare v2, but the only change in the end was removing
this blank line, not worth another round.
>> diff --git a/command-list.txt b/command-list.txt
>> index b17c011..2a94137 100644
>> --- a/command-list.txt
>> +++ b/command-list.txt
>> @@ -148,4 +148,5 @@ git-verify-pack
>> plumbinginterrogators
>> git-verify-tag ancillaryinterrogators
>> gitweb ancillaryinterrogators
>> git-whatchanged ancillaryinterrogators
>> +git-worktreemainporcelain
>
> I doubt that a helper that is primarily spawned from "gc" as its
> implementation detail is more mainporcelain than "git config" is:
>
>git-config ancillarymanipulators
>
> I also wonder if "git worktree" command should have a mode that
> works in a way similar to how new-workdir (in contrib/workdir) does,
> instead of an option "checkout --to" that looks just out of place as
> "worktree prune" was out of place in "prune". The feature is doing
> a lot more than what "checkout" normally does (somewhere in between
> "checkout" and "clone", I would say), and it may be cleaner to use
> an independent command "git worktree" to manage a separate worktree.
>
> And when that happens, the command should definitely be classified
> as a mainporcelain.
And because "git worktree add" is coming, I guess it's ok to stick to
mainporcelain here..
>> diff --git a/git.c b/git.c
>> index 44374b1..fa77bc9 100644
>> --- a/git.c
>> +++ b/git.c
>> @@ -483,6 +483,7 @@ static struct cmd_struct commands[] = {
>> { "verify-tag", cmd_verify_tag, RUN_SETUP },
>> { "version", cmd_version },
>> { "whatchanged", cmd_whatchanged, RUN_SETUP },
>> + { "worktree", cmd_worktree, RUN_SETUP },
>
> We do not NEED_WORK_TREE because we can create a new worktree
> off of a bare repository?
Yes.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] worktree: replace "checkout --to" with "worktree new"
On Tue, Jun 30, 2015 at 11:33 PM, Junio C Hamano wrote: > Duy Nguyen writes: > >> I think this is like "git checkout -b" vs "git branch". We pack so >> many things in 'checkout' that it's a source of both convenience and >> confusion. I never use "git branch" to create a new branch and if I >> had a way to tell checkout to "move away and delete previous branch", >> I would probably stop using "git branch -d/-D" too. "--to" is another >> "-b" in this sense. > > I didn't know "checkout --to" included "create a worktree elsewhere > and chdir there"; if that "and chdir there" is not something you are > doing, then I do not think "checkout -b" vs "branch" analogy applies. Heh.. I do want that "chdir" (even for git-init and git-clone). We can't issue "cd" command back to the parent shell, but we can spawn a new shell with new cwd. But because the target dir is usually at the end of the command line (except for "--to") and "cd !$" is not much to type, it never bothers me enough to do something more. I think this is another reason I prefer "git worktree add" to have the target dir at the end. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] worktree: replace "checkout --to" with "worktree new"
On Thu, Jul 2, 2015 at 12:13 AM, Eric Sunshine wrote: >>I noticed GIT_CHECKOUT_NEW_WORKTREE environment variabl that does >>not seem to be documented. Is this something we still need? >>The log message of 529fef20 (checkout: support checking out into >>a new working directory, 2014-11-30) does not tell us much. > > Yes, it's still used for the same purpose as before the conversion: as > a private signal to the sub git-checkout invocation that it's > operating on a new worktree. When defined, it sets the > 'new_worktree_mode' flag in checkout.c, and there are still a few bits > of code which apparently need to know about it. It would be nice to > eliminate this special knowledge from checkout.c, however, I'm not yet > familiar enough with the checkout code to determine if doing so is > viable. I think it can go away. When "--to" is used, I have to re-execute "git checkout" command again after creating the new worktree. I could process the command line arguments from the first execution, delete "--to", then use the remaining options to run checkout the second time. But I chose to pass the entire command line to the second execution. The env is used to let the second run know it should ignore "--to" (or we get infinite recursion). With "git worktree add" this recursion disappears and this env var has no reason to exist. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] worktree: replace "checkout --to" with "worktree new"
On Thu, Jul 2, 2015 at 9:52 AM, Eric Sunshine wrote:
> On Wed, Jul 1, 2015 at 9:07 PM, Duy Nguyen wrote:
>> On Thu, Jul 2, 2015 at 12:13 AM, Eric Sunshine
>> wrote:
>>>>I noticed GIT_CHECKOUT_NEW_WORKTREE environment variabl that does
>>>>not seem to be documented. Is this something we still need?
>>>>The log message of 529fef20 (checkout: support checking out into
>>>>a new working directory, 2014-11-30) does not tell us much.
>>>
>>> Yes, it's still used for the same purpose as before the conversion: as
>>> a private signal to the sub git-checkout invocation that it's
>>> operating on a new worktree. When defined, it sets the
>>> 'new_worktree_mode' flag in checkout.c, and there are still a few bits
>>> of code which apparently need to know about it. It would be nice to
>>> eliminate this special knowledge from checkout.c, however, I'm not yet
>>> familiar enough with the checkout code to determine if doing so is
>>> viable.
>>
>> I think it can go away. When "--to" is used, I have to re-execute "git
>> checkout" command again after creating the new worktree. I could
>> process the command line arguments from the first execution, delete
>> "--to", then use the remaining options to run checkout the second
>> time. But I chose to pass the entire command line to the second
>> execution. The env is used to let the second run know it should ignore
>> "--to" (or we get infinite recursion). With "git worktree add" this
>> recursion disappears and this env var has no reason to exist.
>
> The recursion protection is indeed no longer needed and gets removed
> by the "worktree add" patch. However, there are still a few bits of
> code which want to know that the checkout is happening in a new
> worktree. I haven't examined them closely yet to diagnose if this
> specialized knowledge can be eliminated. Perhaps you can weight in. In
> particular:
>
> checkout_paths:
> if (opts->new_worktree)
> die(_("'%s' cannot be used with updating paths"), "--to");
This one is easy, as "--to" is gone, no reason to report anything about "--to"
> merge_working_tree:
> tree = parse_tree_indirect(old->commit &&
> !opts->new_worktree_mode ?
> old->commit->object.sha1 :
> EMPTY_TREE_SHA1_BIN);
I think it's to make sure empty sha-1 is used with --to. If
old->commit->object.sha1 is used and it's something, a real two way
merge may happen probably with not-so-fun consequences. If it's empty
sha1, the effect is like "reset --hard", silent and reliable..
> switch_branches:
> if (!opts->quiet && !old.path && old.commit &&
> new->commit != old.commit && !opts->new_worktree_mode)
> orphaned_commit_warning(old.commit, new->commit);
to suppress misleading warning if old.commit happens to be something.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] worktree: replace "checkout --to" with "worktree new"
On Thu, Jul 2, 2015 at 7:41 PM, Duy Nguyen wrote: >> merge_working_tree: >> tree = parse_tree_indirect(old->commit && >> !opts->new_worktree_mode ? >> old->commit->object.sha1 : >> EMPTY_TREE_SHA1_BIN); > > I think it's to make sure empty sha-1 is used with --to. If > old->commit->object.sha1 is used and it's something, a real two way > merge may happen probably with not-so-fun consequences. If it's empty > sha1, the effect is like "reset --hard", silent and reliable.. > >> switch_branches: >> if (!opts->quiet && !old.path && old.commit && >> new->commit != old.commit && !opts->new_worktree_mode) >> orphaned_commit_warning(old.commit, new->commit); > > to suppress misleading warning if old.commit happens to be something. Actually you may be right about not reverting these. We prepare the new worktree with a valid HEAD, that would make "old" valid and may trigger things if "git checkout" is used to populate the worktree. To suppress those "things", we need new_worktree_mode or something similar. Unless we want to borrow fancy checkout options for "git worktree add", we probably should just export checkout() function from clone.c and use it instead of "git checkout". Much more lightweight and simpler (it's one-way merge). Then we can revert checkout.c to the version before "--to". -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] worktree: replace "checkout --to" with "worktree new"
On Fri, Jul 3, 2015 at 12:06 AM, Eric Sunshine wrote:
>> Unless we want to borrow fancy checkout options for "git worktree
>> add", we probably should just export checkout() function from clone.c
>> and use it instead of "git checkout". Much more lightweight and
>> simpler (it's one-way merge). Then we can revert checkout.c to the
>> version before "--to".
>
> Interesting idea, but doesn't this lose the ability to create a new
> branch ("worktree add foo -b bar") and other useful options like
> --track?
Those are what I call "fancy checkout options". I think we could start
simple with clone.c:checkout.c() and maybe libify switch_branches()
later on to gain --detach and stuff. There's another thing I missed,
when the new worktree is set up, HEAD contains a dummy value. It's
expected that the second checkout will update it with proper value (a
ref, detached HEAD). So if you avoid running "git chckout" in
"worktree add" and use clone.c:checkout(), you have to re-do something
similar.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] index-pack: kill union delta_base to save memory
On Fri, Jul 3, 2015 at 11:51 PM, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy writes:
>
>> Once we know the number of objects in the input pack, we allocate an
>> array of nr_objects of struct delta_entry. On x86-64, this struct is
>> 32 bytes long. The union delta_base, which is part of struct
>> delta_entry, provides enough space to store either ofs-delta (8 bytes)
>> or ref-delta (20 bytes).
>
> Sorry for responding to a patch 7000+ messages ago, but some kind
> folks at Google were puzzled by this code, and I think they found a
> small bug.
>
>> static void fix_unresolved_deltas(struct sha1file *f, int nr_unresolved)
>> {
>> - struct delta_entry **sorted_by_pos;
>> + struct ref_delta_entry **sorted_by_pos;
>> int i, n = 0;
>>
>> /*
>> @@ -1282,28 +1344,25 @@ static void fix_unresolved_deltas(struct sha1file
>> *f, int nr_unresolved)
>>* resolving deltas in the same order as their position in the pack.
>>*/
>> sorted_by_pos = xmalloc(nr_unresolved * sizeof(*sorted_by_pos));
>> - for (i = 0; i < nr_deltas; i++) {
>> - if (objects[deltas[i].obj_no].real_type != OBJ_REF_DELTA)
>> - continue;
>> - sorted_by_pos[n++] = &deltas[i];
>> - }
>> + for (i = 0; i < nr_ref_deltas; i++)
>> + sorted_by_pos[n++] = &ref_deltas[i];
>> qsort(sorted_by_pos, n, sizeof(*sorted_by_pos), delta_pos_compare);
>
> You allocate an array of nr_unresolved (which is the sum of
> nr_ref_deltas and nr_ofs_deltas in the new world order with patch)
> entries, fill only the first nr_ref_deltas entries of it, and then
> sort that first n (= nr_ref_deltas) entries. The qsort and the
> subsequent loop only looks at the first n entries, so nothing is
> filling or reading these nr_ofs_deltas entres at the end.
>
> I do not see any wrong behaviour other than temporary wastage of
> nr_ofs_deltas pointers that would be caused by this, but this
> allocation is misleading.
>
> Also, the old code before this change had to use 'i' and 'n' because
> some of the things we see in the (old) deltas[] array we scanned
> with 'i' would not make it into the sorted_by_pos[] array in the old
> world order, but now because you have only ref delta in a separate
> ref_deltas[] array, they increment lock&step. That also puzzled me
> while re-reading this code.
>
> Perhaps something like this is needed?
Yeah I can see the confusion when reading the code without checking
its history. You probably want to kill the argument nr_unresolved too
because it's not needed anymore after your patch.
So what's the bug you mentioned? All I got from the above was wastage
and confusion, no bug.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/23] Documentation/git-worktree: add BUGS section
On Sat, Jul 4, 2015 at 7:17 AM, Eric Sunshine wrote: > +git-worktree could provide more automation for tasks currently > +performed manually or via other commands, such as: > + > +- `add` to create a new linked worktree > +- `remove` to remove a linked worktree and its administrative files (and > + warn if the worktree is dirty) > +- `mv` to move or rename a worktree and update its administrative files > +- `lock` to prevent automatic pruning of administrative files (for instance, > + for a worktree on a portable device) No need to re-roll if this is the only change in the series, but we also need "list/ls" here. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 08/23] checkout: fix bug with --to and relative HEAD
On Sat, Jul 4, 2015 at 7:17 AM, Eric Sunshine wrote:
> Given "git checkout --to HEAD~1", the new worktree's HEAD should
> begin life at the current branch's HEAD~1, however, it actually ends up
> at HEAD~2. The happens because:
>
> 1. git-checkout resolves HEAD~1
>
> 2. to satisfy is_git_directory, prepare_linked_worktree() creates a
>HEAD in the new worktree with the value of the resolved HEAD~1
>
> 3. git-checkout re-invokes itself with the same arguments within the
>new worktree to populate the worktree
>
> 4. the sub git-checkout resolves HEAD~1 relative to its own HEAD,
>which is the resolved HEAD~1 from the original invocation,
>resulting unexpectedly and incorrectly in HEAD~2 (relative to the
>original)
>
> Fix this by unconditionally assigning the current worktree's HEAD as the
> value of the new worktree's HEAD.
Good catch!
> @@ -924,9 +925,11 @@ static int prepare_linked_checkout(const struct
> checkout_opts *opts,
> * value would do because this value will be ignored and
> * replaced at the next (real) checkout.
> */
This comment "any valid value would do.. will be ignored" is proved incorrect.
> + if (!resolve_ref_unsafe("HEAD", 0, rev, NULL))
> + die(_("unable to resolve HEAD"));
> strbuf_reset(&sb);
> strbuf_addf(&sb, "%s/HEAD", sb_repo.buf);
> - write_file(sb.buf, 1, "%s\n", sha1_to_hex(new->commit->object.sha1));
> + write_file(sb.buf, 1, "%s\n", sha1_to_hex(rev));
> strbuf_reset(&sb);
> strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
> write_file(sb.buf, 1, "../..\n");
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 13/23] worktree: introduce "add" command
On Sat, Jul 4, 2015 at 7:17 AM, Eric Sunshine wrote: > COMMANDS > > +add :: > + > +Check out `` into a separate working directory, ``, creating > +`` if necessary. The new working directory is linked to the current > +repository, sharing everything except working directory specific files > +such as HEAD, index, etc. If `` already exists, it must be empty. Side note, "must be empty" is an implementation limitation. I think the two-way merge employed by git-checkout can deal with dirty and only perform the checkout if there is no data loss. But we can leave this for later. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 18/23] checkout: retire --to option
On Sat, Jul 4, 2015 at 7:17 AM, Eric Sunshine wrote: > Now that "git worktree add" has achieved user-facing feature-parity with > "git checkout --to", retire the latter. > > Move the actual linked worktree creation functionality, > prepare_linked_checkout() and its helpers, verbatim from checkout.c to > worktree.c. > > This effectively reverts changes to checkout.c by 529fef2 (checkout: > support checking out into a new working directory, 2014-11-30) with the > exception of merge_working_tree() and switch_branches() which still > require specialized knowledge that a the checkout is occurring in a > newly-created linked worktree (signaled to them by the private > GIT_CHECKOUT_NEW_WORKTREE environment variable). > > Signed-off-by: Eric Sunshine > --- > builtin/checkout.c | 156 > + > builtin/worktree.c | 138 --- > 2 files changed, 133 insertions(+), 161 deletions(-) If I didn't lose track of changes, "--to" is still described in git-checkout.txt? -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
