* Re: [PATCH] diff: fix up SHA-1 abbreviations outside of repository
From: Junio C Hamano @ 2016-12-05 22:43 UTC (permalink / raw)
To: Jeff King; +Cc: Jack Bates, git, Jack Bates
In-Reply-To: <20161205072614.zg6yglqnznna65vf@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I agree that it may be an accident waiting to happen, though, as soon as
> some buried sub-function needs to care about the distinction.
Yes to that.
>> I wonder if we're better off if we made sure that diff_no_index()
>> works the same way regardless of the value of "have_repository"
>> field?
>
> If you mean adding a diffopt flag like "just abbreviate everything to
> FALLBACK_DEFAULT_ABBREV even if we're in a repository", and then setting
> that in diff_no_index(), I agree that is a lot cleaner.
I am not sure if that is what I meant (I no longer sure what exactly
I meant to say there TBH), but this is probably not limited to the
default abbrev length aka core.abbrev configuration. Don't we have
other configuration settings we may read from $HOME/.gitconfig (and
possibly per-repository .git/config, if we did discovery but were
explicitly given "--no-index") that want to affect the behaviour of
the command?
I guess what I wanted, with "the same way", to see happen was that
"have_repository" should be only controling how and from what files
the configuration is read, and the behaviour of the command should
be controlled by the values read from the configuration after that.
Specifically, even if we were running with "--no-index", if we know
we have access to the current repository discovered by setting it up
gently, I do not think it is bad to ask find_unique_abbrev() to come
up with an appropriate abbreviation. So the fact that patch in
question has to flip the have_repository bit off, if it is done in
order to affect what diff_abbrev_oid() does, smells quite fishy from
that point of view.
^ permalink raw reply
* Re: Feature request: Set git svn options in .git/config file
From: Eric Wong @ 2016-12-05 22:54 UTC (permalink / raw)
To: Juergen Kosel; +Cc: git
In-Reply-To: <1936940c-c4c8-540c-eb99-b434e8d32d6c@gmx.de>
Juergen Kosel <juergen.kosel@gmx.de> wrote:
> Therefore I believe, that it would be the best solution to store the
> settings of --add-author-from, --use-log-author and maybe
> --authors-prog in the .git/config file.
Actually, "svn.authorsProg" is already documented as a config
option for --authors-prog.
It's been a while since I looked at this, but in git-svn,
all the "--xxx-yyy" command-line options should be available
under the appropriate "svn.xxxYyy" config key.
So, can you confirm that svn.addAuthorFrom and svn.useLogAuthor
config keys work and can be documented?
Even better would be for you to provide a patch to the
documentation :)
Otherwise, I can write one up sometime this week.
Thanks.
^ permalink raw reply
* Re: [PATCHv1 1/2] git-p4: support git-workspaces
From: Junio C Hamano @ 2016-12-05 23:17 UTC (permalink / raw)
To: Luke Diamand; +Cc: Git Users, Vinicius Kursancew, Lars Schneider
In-Reply-To: <CAE5ih79efEu_2jgE9V-N7+UetyYu7RjH62whcfvMBtwM-Nb8Sg@mail.gmail.com>
Luke Diamand <luke@diamand.org> writes:
>> What I am trying to get at is if we want to use a single command
>> that can be given a path and answer "Yes, that is a repository"
>> here, and that single command should know how the repository should
>> look like. I offhand do not know we already have such a command we
>> can use, e.g. "git rev-parse --is-git-dir $path", but if there isn't
>> perhaps we would want one, so that not just "git p4" but other
>> scripted Porcelains can make use of it?
>
> That would be nicer than random ad-hoc rules, certainly. I couldn't
> find any obvious combination that would work.
I've already sent an update but my reading of this code is that
there is no need for the program to be able to, given a random
directory path, ask isValidGitDir() "is this a Git repository?" at
all.
What the program wanted to know, IIUC, is "Where is the 'Git
repository directory', if there is one?". This is needed primarily
because the program wants to error out before doing any operation
that requires a Git repository to work on, when the answer is "there
is none".
It also wants to know it because (for whatever reason) it wants to
export it in GIT_DIR [*1*].
And isValidGitDir() is a helper function used by the handcrafted
logic in main() to answer that question, but as far as I can tell,
you'd get a more correct answer by asking "rev-parse --git-dir"
unconditionally (even when the user of the program exported GIT_DIR
to you).
I just was reading Lars's LFS changes, but that one has hardcoded
"Is there a '.git/lfs/objects/' directory directly inside the
current working directory?" and similar, which I do not think would
work well in a secondary working tree where ".git" is merely a file
[*2*]. In a "secondary working tree"-aware world, I think you would
need to ask locations of --git-dir and --git-common-dir to rev-parse
upfront and choose which ones are ought to be shared entities across
the family of worktrees and which ones are to be private per
worktree. I suspect that LFS objects want to be shared across
working trees that share the object store, for example, which would
mean that "--git-dir" is not what Lars would want to use.
[Footnote]
*1* I do not think this is necessary. Git tools know how to find
the repository by first checking GIT_DIR environment and if it is
not set then by walking up the directory hierarchy just fine without
the program exporting GIT_DIR for them.
*2* The part that bases this path relative to getcwd() is OK, as the
start-up sequence in main() does a cdup to the top before everything
else.
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Junio C Hamano @ 2016-12-05 23:19 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jeff King, git, sbeller, bburky, jrnieder
In-Reply-To: <20161205222211.GF68588@google.com>
Brandon Williams <bmwill@google.com> writes:
> I just took Jeff's series and applied it on top of mine (and fixed the
> small problem causing t5812 to fail) and then rebased it to v2.9.0.
> There were a few issues that needed to be resolved but nothing too
> hairy. So it would definitely be doable to backport it to the
> maintenance tracks.
Thanks for trying. I'd definitely prefer a series that is based on
an older codebase that is merged to pu->next->master->maint (in
other words, avoid "backport" and rather have "forward merge").
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-05 23:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, sbeller, bburky, jrnieder
In-Reply-To: <xmqqa8ca2cc7.fsf@gitster.mtv.corp.google.com>
On 12/05, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > I just took Jeff's series and applied it on top of mine (and fixed the
> > small problem causing t5812 to fail) and then rebased it to v2.9.0.
> > There were a few issues that needed to be resolved but nothing too
> > hairy. So it would definitely be doable to backport it to the
> > maintenance tracks.
>
> Thanks for trying. I'd definitely prefer a series that is based on
> an older codebase that is merged to pu->next->master->maint (in
> other words, avoid "backport" and rather have "forward merge").
Ah ok. Do you want me to send out the combined patch series based on
2.9.0 (or some other point in time) then?
--
Brandon Williams
^ permalink raw reply
* RE: [RFC PATCHv2 15/17] checkout: recurse into submodules if asked to
From: David Turner @ 2016-12-05 23:36 UTC (permalink / raw)
To: 'Stefan Beller', bmwill@google.com
Cc: git@vger.kernel.org, sandals@crustytoothpaste.net,
hvoigt@hvoigt.net, gitster@pobox.com
In-Reply-To: <20161203003022.29797-16-sbeller@google.com>
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Friday, December 02, 2016 7:30 PM
> To: bmwill@google.com; David Turner
> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; hvoigt@hvoigt.net;
> gitster@pobox.com; Stefan Beller
> Subject: [RFC PATCHv2 15/17] checkout: recurse into submodules if asked to
>
> Allow checkout to recurse into submodules via the command line option --
> [no-]recurse-submodules.
This option probably needs to precede 9/17 "update submodules: add scheduling to update submodules", since that patch uses --recurse-submodules.
^ permalink raw reply
* Re: git describe number of commits different from git log count
From: Aaron Schrab @ 2016-12-05 23:27 UTC (permalink / raw)
To: Jan Hudec; +Cc: git
In-Reply-To: <1701823.75Syo8h0k0@box>
At 12:17 +0100 05 Feb 2016, Jan Hudec <bulb@ucw.cz> wrote:
>I have a repository with following situation:
>
> $ git describe next
> v4.1-2196-g5a414d7
> $ git describe next --match=v4.2
> v4.2-4757-g5a414d7
>
>Since the tag with fewest commits since is selected, it appears logical.
>However, v4.2 is descendant of v4.1, so it does not make sense for it to have
>more commits since. And rev-list or log confirm that:
>
> $ git rev-list v4.1..next | wc -l
> 2196
> $ git rev-list v4.2..next | wc -l
> 1152
>
>The number of commits since v4.1 matches what the describe counted, but the
>number of commits since v4.2 does not.
I'm encountering what seems to be a similar issue. I'm working with the
`build` branch of https://github.com/aschrab/mutt currently at 65b7094.
Most of the commits in that repo come from a mercurial repository, but I
don't think that is really effecting things (other than being related to
the need to use the --tags option).
$ git describe --tags
mutt-1-7-1-rel-137-g65b7094
$ git describe --tags --match=mutt-1-7-2-rel
mutt-1-7-2-rel-6246-g65b7094
$ git rev-list --count mutt-1-7-2-rel..HEAD
126
$ git rev-list --count mutt-1-7-1-rel..HEAD
137
$ git --version
git version 2.10.2
The number of additional commits shown when I force the tag is
completely insane! That's nearly every commit that is part of that
branch:
1036$ git rev-list --count HEAD
6250
Both of the tags above should be reachable along the first-parent path.
If I add the `--first-parent` option to the describe command it picks
the expected tag and the number additional commits seems sane:
$ git describe --tags --first-parent
mutt-1-7-2-rel-14-g65b7094
^ permalink raw reply
* RE: [RFC PATCHv2 09/17] update submodules: add scheduling to update submodules
From: David Turner @ 2016-12-05 23:37 UTC (permalink / raw)
To: 'Stefan Beller', bmwill@google.com
Cc: git@vger.kernel.org, sandals@crustytoothpaste.net,
hvoigt@hvoigt.net, gitster@pobox.com
In-Reply-To: <20161203003022.29797-10-sbeller@google.com>
This patch confuses me -- see below.
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Friday, December 02, 2016 7:30 PM
> To: bmwill@google.com; David Turner
> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; hvoigt@hvoigt.net;
> gitster@pobox.com; Stefan Beller
> Subject: [RFC PATCHv2 09/17] update submodules: add scheduling to update
> submodules
[snip]
> +static int update_submodule(const char *path, const struct object_id
> *oid,
> + int force, int is_new)
> +{
> + const char *git_dir;
> + struct child_process cp = CHILD_PROCESS_INIT;
> + const struct submodule *sub = submodule_from_path(null_sha1, path);
> +
> + if (!sub || !sub->name)
> + return -1;
> +
> + git_dir = resolve_gitdir(git_common_path("modules/%s", sub->name));
> +
> + if (!git_dir)
> + return -1;
> +
> + if (is_new)
> + connect_work_tree_and_git_dir(path, git_dir);
> +
> + /* update index via `read-tree --reset sha1` */
> + argv_array_pushl(&cp.args, "read-tree",
> + force ? "--reset" : "-m",
> + "-u", sha1_to_hex(oid->hash), NULL);
> + prepare_submodule_repo_env(&cp.env_array);
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.dir = path;
> + if (run_command(&cp)) {
> + warning(_("reading the index in submodule '%s' failed"),
> path);
The error is not (usually) in "reading the index" -- it's "updating the index" (or the working tree)
> + child_process_clear(&cp);
> + return -1;
> + }
> +
> + /* write index to working dir */
> + child_process_clear(&cp);
> + child_process_init(&cp);
> + argv_array_pushl(&cp.args, "checkout-index", "-a", NULL);
I'm confused -- doesn't read-tree -u already do this? And if not, shouldn't we back out the result of the read-tree, to leave the submodule as it was?
> + cp.git_cmd = 1;
> + cp.no_stdin = 1;
> + cp.dir = path;
> + if (force)
> + argv_array_push(&cp.args, "-f");
> +
> + if (run_command(&cp)) {
> + warning(_("populating the working directory in submodule '%s'
> failed"), path);
> + child_process_clear(&cp);
> + return -1;
> + }
> +
> + /* get the HEAD right */
> + child_process_clear(&cp);
> + child_process_init(&cp);
> + argv_array_pushl(&cp.args, "checkout", "--recurse-submodules",
> NULL);
Why are we running checkout on the submodule when we've already done most of the checkout? The only thing left is to set HEAD and recurse, right? I must be missing something.
^ permalink raw reply
* RE: [RFC PATCHv2 14/17] submodule: teach unpack_trees() to update submodules
From: David Turner @ 2016-12-05 23:37 UTC (permalink / raw)
To: 'Stefan Beller', bmwill@google.com
Cc: git@vger.kernel.org, sandals@crustytoothpaste.net,
hvoigt@hvoigt.net, gitster@pobox.com
In-Reply-To: <20161203003022.29797-15-sbeller@google.com>
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Friday, December 02, 2016 7:30 PM
> To: bmwill@google.com; David Turner
> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; hvoigt@hvoigt.net;
> gitster@pobox.com; Stefan Beller
> Subject: [RFC PATCHv2 14/17] submodule: teach unpack_trees() to update
> submodules
[snip]
> if (!lstat(ce->name, &st)) {
> - int flags =
> CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;
> - unsigned changed = ie_match_stat(o->src_index, ce, &st,
> flags);
> - if (!changed)
> - return 0;
> - /*
> - * NEEDSWORK: the current default policy is to allow
> - * submodule to be out of sync wrt the superproject
> - * index. This needs to be tightened later for
> - * submodules that are marked to be automatically
> - * checked out.
> - */
> - if (S_ISGITLINK(ce->ce_mode))
> - return 0;
> + if (S_ISGITLINK(ce->ce_mode)) {
> + if (!submodule_is_interesting(ce->name))
> + return 0;
> + if (ce_stage(ce) ? is_submodule_checkout_safe(ce->name,
> &ce->oid)
> + : !is_submodule_modified(ce->name, 1))
This logic is too convoluted. Do a nested if or something.
> + return 0;
> + } else {
> + int flags = CE_MATCH_IGNORE_VALID |
> CE_MATCH_IGNORE_SKIP_WORKTREE;
> + if (!ie_match_stat(o->src_index, ce, &st, flags))
> + return 0;
Nit: I liked the old temp var "changed" better -- it made it clear what
ie_match_stat is checking.
> + }
> errno = 0;
> }
> if (errno == ENOENT)
> @@ -1355,6 +1359,38 @@ static int verify_uptodate_sparse(const struct
> cache_entry *ce,
> return verify_uptodate_1(ce, o, ERROR_SPARSE_NOT_UPTODATE_FILE); }
>
> +/*
> + * When a submodule gets turned into an unmerged entry, we want it to
> +be
> + * up-to-date regarding the merge changes.
> + */
> +static int verify_uptodate_submodule(const struct cache_entry *old,
> + const struct cache_entry *new,
> + struct unpack_trees_options *o) {
> + struct stat st;
> +
> + if (o->index_only ||
> + (!((old->ce_flags & CE_VALID) || ce_skip_worktree(old)) &&
> + (o->reset || ce_uptodate(old))))
> + return 0;
> +
> + if (!submodule_is_interesting(new->name))
> + return 0;
> +
> + if (lstat(old->name, &st)) {
We never actually use st here. Should we? If not, is this really the right error message? And should we use access() instead of lstat?
> + if (errno == ENOENT)
> + return 0;
> + return o->gently ? -1 :
> + add_rejected_path(o, ERROR_NOT_UPTODATE_SUBMODULE,
> + old->name);
> + }
> +
^ permalink raw reply
* RE: [RFC PATCHv2 08/17] update submodules: add depopulate_submodule
From: David Turner @ 2016-12-05 23:37 UTC (permalink / raw)
To: 'Stefan Beller', bmwill@google.com
Cc: git@vger.kernel.org, sandals@crustytoothpaste.net,
hvoigt@hvoigt.net, gitster@pobox.com
In-Reply-To: <20161203003022.29797-9-sbeller@google.com>
> -----Original Message-----
> From: Stefan Beller [mailto:sbeller@google.com]
> Sent: Friday, December 02, 2016 7:30 PM
> To: bmwill@google.com; David Turner
> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; hvoigt@hvoigt.net;
> gitster@pobox.com; Stefan Beller
> Subject: [RFC PATCHv2 08/17] update submodules: add depopulate_submodule
>
> Implement the functionality needed to enable work tree manipulating
> commands so that a deleted submodule should not only affect the index
> (leaving all the files of the submodule in the work tree) but also to
> remove the work tree of the superproject (including any untracked files).
"including any untracked files" bothers me, I think. Checkout is not usually willing to overwrite untracked files; it seems odd to me that it would be willing to do so in the submodule case. I would be OK if they were both untracked and gitignored, I think.
> To do so, we need an equivalent of "rm -rf", which is already found in
> entry.c, so expose that and for clarity add a suffix "_or_dir" to it.
>
> That will only work properly when the submodule uses a gitfile instead of
> a .git directory and no untracked files are present. Otherwise the removal
> will fail with a warning (which is just what happened until now).
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> cache.h | 2 ++
> entry.c | 5 +++++
> submodule.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> submodule.h | 1 +
> 4 files changed, 54 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index a50a61a197..b645ca2f9a 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -2018,4 +2018,6 @@ void sleep_millisec(int millisec);
> */
> void safe_create_dir(const char *dir, int share);
>
> +extern void remove_directory_or_die(struct strbuf *path);
> +
> #endif /* CACHE_H */
> diff --git a/entry.c b/entry.c
> index c6eea240b6..02c4ac9f22 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -73,6 +73,11 @@ static void remove_subtree(struct strbuf *path)
> die_errno("cannot rmdir '%s'", path->buf); }
>
> +void remove_directory_or_die(struct strbuf *path) {
> + remove_subtree(path);
> +}
> +
> static int create_file(const char *path, unsigned int mode) {
> mode = (mode & 0100) ? 0777 : 0666;
> diff --git a/submodule.c b/submodule.c
> index 62e9ef3872..7bb64d6c69 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -324,6 +324,52 @@ void prepare_submodule_repo_env(struct argv_array
> *out)
> argv_array_push(out, "GIT_DIR=.git");
> }
>
> +int depopulate_submodule(const char *path) {
> + int ret = 0;
> + struct strbuf pathbuf = STRBUF_INIT;
> + char *dot_git = xstrfmt("%s/.git", path);
> +
> + /* Is it populated? */
> + if (!resolve_gitdir(dot_git))
> + goto out;
> +
> + /* Does it have a .git directory? */
> + if (!submodule_uses_gitfile(path)) {
> + struct child_process cp = CHILD_PROCESS_INIT;
> +
> + prepare_submodule_repo_env(&cp.env_array);
> + argv_array_pushl(&cp.args, "submodule--helper",
> + "embed-git-dirs", path, NULL);
> + cp.git_cmd = 1;
> + if (run_command(&cp)) {
> + warning(_("Cannot remove submodule '%s'\n"
> + "because it (or one of its nested submodules)
> has a git \n"
> + "directory in the working tree, which could not
> be embedded\n"
> + "the superprojects git directory
> automatically."), path);
What if instead it couldn't run the command because you're out of file descriptors or pids or memory or something?
I think this message should be in submodule--helper --embed-git-dirs instead, and we should just pass it through here. Or, perhaps, instead of shelling out here, we should just call the functions directly?
> + ret = -1;
> + goto out;
> + }
> +
> + if (!submodule_uses_gitfile(path)) {
> + /*
> + * We should be using a gitfile by now, let's double
Comma splice.
> + * check as loosing the git dir would be fatal.
s/loosing/losing/
> + */
> + die("BUG: \"git submodule--helper embed git-dirs '%s'\"
> "
> + "did not embed the git-dirs recursively for '%s'",
> + path, path);
> + }
> + }
> +
> + strbuf_addstr(&pathbuf, path);
> + remove_directory_or_die(&pathbuf);
> +out:
> + strbuf_release(&pathbuf);
> + free(dot_git);
> + return ret;
> +}
> +
> /* Helper function to display the submodule header line prior to the full
> * summary output. If it can locate the submodule objects directory it
> will
> * attempt to lookup both the left and right commits and put them into
> the diff --git a/submodule.h b/submodule.h index 7d890e0464..d8bb1d4baf
> 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -63,6 +63,7 @@ extern void set_config_update_recurse_submodules(int
> value);
> */
> extern int submodule_is_interesting(const char *path); extern int
> submodules_interesting_for_update(void);
> +extern int depopulate_submodule(const char *path);
> extern void check_for_new_submodule_commits(unsigned char new_sha1[20]);
> extern int fetch_populated_submodules(const struct argv_array *options,
> const char *prefix, int command_line_option,
> --
> 2.11.0.rc2.28.g2673dad
^ permalink raw reply
* Re: [RFC PATCHv2 09/17] update submodules: add scheduling to update submodules
From: Stefan Beller @ 2016-12-05 23:54 UTC (permalink / raw)
To: David Turner
Cc: bmwill@google.com, git@vger.kernel.org,
sandals@crustytoothpaste.net, hvoigt@hvoigt.net,
gitster@pobox.com
In-Reply-To: <f19844d15ab4424b8c056cd13837d233@exmbdft7.ad.twosigma.com>
On Mon, Dec 5, 2016 at 3:37 PM, David Turner <David.Turner@twosigma.com> wrote:
> This patch confuses me -- see below.
>
>> -----Original Message-----
>> From: Stefan Beller [mailto:sbeller@google.com]
>> Sent: Friday, December 02, 2016 7:30 PM
>> To: bmwill@google.com; David Turner
>> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; hvoigt@hvoigt.net;
>> gitster@pobox.com; Stefan Beller
>> Subject: [RFC PATCHv2 09/17] update submodules: add scheduling to update
>> submodules
> [snip]
>> +static int update_submodule(const char *path, const struct object_id
>> *oid,
>> + int force, int is_new)
>> +{
>> + const char *git_dir;
>> + struct child_process cp = CHILD_PROCESS_INIT;
>> + const struct submodule *sub = submodule_from_path(null_sha1, path);
>> +
>> + if (!sub || !sub->name)
>> + return -1;
>> +
>> + git_dir = resolve_gitdir(git_common_path("modules/%s", sub->name));
>> +
>> + if (!git_dir)
>> + return -1;
>> +
>> + if (is_new)
>> + connect_work_tree_and_git_dir(path, git_dir);
>> +
>> + /* update index via `read-tree --reset sha1` */
>> + argv_array_pushl(&cp.args, "read-tree",
>> + force ? "--reset" : "-m",
>> + "-u", sha1_to_hex(oid->hash), NULL);
>> + prepare_submodule_repo_env(&cp.env_array);
>> + cp.git_cmd = 1;
>> + cp.no_stdin = 1;
>> + cp.dir = path;
>> + if (run_command(&cp)) {
>> + warning(_("reading the index in submodule '%s' failed"),
>> path);
>
> The error is not (usually) in "reading the index" -- it's "updating the index" (or the working tree)
>
>> + child_process_clear(&cp);
>> + return -1;
>> + }
>> +
>> + /* write index to working dir */
>> + child_process_clear(&cp);
>> + child_process_init(&cp);
>> + argv_array_pushl(&cp.args, "checkout-index", "-a", NULL);
>
> I'm confused -- doesn't read-tree -u already do this? And if not, shouldn't we back out the result of the read-tree, to leave the submodule as it was?
>
>> + cp.git_cmd = 1;
>> + cp.no_stdin = 1;
>> + cp.dir = path;
>> + if (force)
>> + argv_array_push(&cp.args, "-f");
>> +
>> + if (run_command(&cp)) {
>> + warning(_("populating the working directory in submodule '%s'
>> failed"), path);
>> + child_process_clear(&cp);
>> + return -1;
>> + }
>> +
>> + /* get the HEAD right */
>> + child_process_clear(&cp);
>> + child_process_init(&cp);
>> + argv_array_pushl(&cp.args, "checkout", "--recurse-submodules",
>> NULL);
>
>
> Why are we running checkout on the submodule when we've already done most of the checkout? The only thing left is to set HEAD and recurse, right? I must be missing something.
>
Yes this is only used to set the HEAD correctly and then recurse down.
I tried to remove the first 2 calls to ch8ild processes at one point in time,
which did not work out. I should have written in the commit message why
that was a problem. So I'll redo that just to see the problem and improve
the commit message.
^ permalink raw reply
* Re: [RFC PATCHv2 08/17] update submodules: add depopulate_submodule
From: Stefan Beller @ 2016-12-06 0:18 UTC (permalink / raw)
To: David Turner
Cc: bmwill@google.com, git@vger.kernel.org,
sandals@crustytoothpaste.net, hvoigt@hvoigt.net,
gitster@pobox.com
In-Reply-To: <832dcc3eec0d4237a1e2766e8df690ee@exmbdft7.ad.twosigma.com>
On Mon, Dec 5, 2016 at 3:37 PM, David Turner <David.Turner@twosigma.com> wrote:
>> -----Original Message-----
>> From: Stefan Beller [mailto:sbeller@google.com]
>> Sent: Friday, December 02, 2016 7:30 PM
>> To: bmwill@google.com; David Turner
>> Cc: git@vger.kernel.org; sandals@crustytoothpaste.net; hvoigt@hvoigt.net;
>> gitster@pobox.com; Stefan Beller
>> Subject: [RFC PATCHv2 08/17] update submodules: add depopulate_submodule
>>
>> Implement the functionality needed to enable work tree manipulating
>> commands so that a deleted submodule should not only affect the index
>> (leaving all the files of the submodule in the work tree) but also to
>> remove the work tree of the superproject (including any untracked files).
>
> "including any untracked files" bothers me, I think. Checkout is not usually willing to overwrite untracked files; it seems odd to me that it would be willing to do so in the submodule case. I would be OK if they were both untracked and gitignored, I think.
I agree on being bothered, this is one of the things I thought how to solve.
See the test in "checkout: recurse into submodules if asked to", which
tests for untracked files and is still marked as a failure.
I think to address that issue, I'll add a flag to ok_to_remove_submodule
which let's you specify which files you care about and which you don't.
>> + warning(_("Cannot remove submodule '%s'\n"
>> + "because it (or one of its nested submodules)
>> has a git \n"
>> + "directory in the working tree, which could not
>> be embedded\n"
>> + "the superprojects git directory
>> automatically."), path);
>
> What if instead it couldn't run the command because you're out of file descriptors or pids or memory or something?
>
> I think this message should be in submodule--helper --embed-git-dirs instead, and we should just pass it through here. Or, perhaps, instead of shelling out here, we should just call the functions directly?
heh, good point. Will call the function directly.
>
^ permalink raw reply
* [PATCH v3] diff: handle --no-abbrev in no-index case
From: Jack Bates @ 2016-12-06 1:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Jack Bates
In-Reply-To: <20161205065823.c7qw6xtc2hqk3xgu@sigill.intra.peff.net>
There are two different places where the --no-abbrev option is parsed,
and two different places where SHA-1s are abbreviated. We normally parse
--no-abbrev with setup_revisions(), but in the no-index case, "git diff"
calls diff_opt_parse() directly, and diff_opt_parse() didn't handle
--no-abbrev until now. (It did handle --abbrev, however.) We normally
abbreviate SHA-1s with find_unique_abbrev(), but commit 4f03666 ("diff:
handle sha1 abbreviations outside of repository, 2016-10-20) recently
introduced a special case when you run "git diff" outside of a
repository.
setup_revisions() does also call diff_opt_parse(), but not for --abbrev
or --no-abbrev, which it handles itself. setup_revisions() sets
rev_info->abbrev, and later copies that to diff_options->abbrev. It
handles --no-abbrev by setting abbrev to zero. (This change doesn't
touch that.)
Setting abbrev to zero was broken in the outside-of-a-repository special
case, which until now resulted in a truly zero-length SHA-1, rather than
taking zero to mean do not abbreviate. The only way to trigger this bug,
however, was by running "git diff --raw" without either the --abbrev or
--no-abbrev options, because 1) without --raw it doesn't respect abbrev
(which is bizarre, but has been that way forever), 2) we silently clamp
--abbrev=0 to MINIMUM_ABBREV, and 3) --no-abbrev wasn't handled until
now.
The outside-of-a-repository case is one of three no-index cases. The
other two are when one of the files you're comparing is outside of the
repository you're in, and the --no-index option.
Signed-off-by: Jack Bates <jack@nottheoilrig.com>
---
diff.c | 6 +++++-
t/t4013-diff-various.sh | 7 +++++++
t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir | 3 +++
t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++
t/t4013/diff.diff_--no-index_--raw_dir2_dir | 3 +++
t/t4013/diff.diff_--raw_--abbrev=4_initial | 6 ++++++
t/t4013/diff.diff_--raw_--no-abbrev_initial | 6 ++++++
t/t4013/diff.diff_--raw_initial | 6 ++++++
8 files changed, 39 insertions(+), 1 deletion(-)
create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir
create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial
create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial
create mode 100644 t/t4013/diff.diff_--raw_initial
diff --git a/diff.c b/diff.c
index ec87283..84dba60 100644
--- a/diff.c
+++ b/diff.c
@@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)
abbrev = FALLBACK_DEFAULT_ABBREV;
if (abbrev > GIT_SHA1_HEXSZ)
die("BUG: oid abbreviation out of range: %d", abbrev);
- hex[abbrev] = '\0';
+ if (abbrev)
+ hex[abbrev] = '\0';
return hex;
}
}
@@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)
options->file = stdout;
+ options->abbrev = DEFAULT_ABBREV;
options->line_termination = '\n';
options->break_opt = -1;
options->rename_limit = -1;
@@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,
offending, optarg);
return argcount;
}
+ else if (!strcmp(arg, "--no-abbrev"))
+ options->abbrev = 0;
else if (!strcmp(arg, "--abbrev"))
options->abbrev = DEFAULT_ABBREV;
else if (skip_prefix(arg, "--abbrev=", &arg)) {
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 566817e..d7b71a0 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side
diff --dirstat master~1 master~2
diff --dirstat initial rearrange
diff --dirstat-by-file initial rearrange
+# --abbrev and --no-abbrev outside of repository
+diff --raw initial
+diff --raw --abbrev=4 initial
+diff --raw --no-abbrev initial
+diff --no-index --raw dir2 dir
+diff --no-index --raw --abbrev=4 dir2 dir
+diff --no-index --raw --no-abbrev dir2 dir
EOF
test_expect_success 'log -S requires an argument' '
diff --git a/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
new file mode 100644
index 0000000..a71b38a
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --abbrev=4 dir2 dir
+:000000 100644 0000... 0000... A dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
new file mode 100644
index 0000000..e0f0097
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --no-abbrev dir2 dir
+:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
new file mode 100644
index 0000000..3cb4ee7
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw dir2 dir
+:000000 100644 0000000... 0000000... A dir/sub
+$
diff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial
new file mode 100644
index 0000000..c3641db
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --abbrev=4 initial
+:100644 100644 35d2... 9929... M dir/sub
+:100644 100644 01e7... 10a8... M file0
+:000000 100644 0000... b1e6... A file1
+:100644 000000 01e7... 0000... D file2
+$
diff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial
new file mode 100644
index 0000000..c87a125
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --no-abbrev initial
+:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M dir/sub
+:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M file0
+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A file1
+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D file2
+$
diff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial
new file mode 100644
index 0000000..a3e9780
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_initial
@@ -0,0 +1,6 @@
+$ git diff --raw initial
+:100644 100644 35d242b... 992913c... M dir/sub
+:100644 100644 01e79c3... 10a8a9f... M file0
+:000000 100644 0000000... b1e6722... A file1
+:100644 000000 01e79c3... 0000000... D file2
+$
--
2.10.2
^ permalink raw reply related
* [PATCH] git-p4: add p4 shelf support
From: Nuno Subtil @ 2016-12-06 2:02 UTC (permalink / raw)
To: git
Extends the submit command to support shelving a commit instead of
submitting it to p4 (similar to --prepare-p4-only).
Signed-off-by: Nuno Subtil <subtil@gmail.com>
---
git-p4.py | 36 ++++++++++++++++++++++++++++++------
1 file changed, 30 insertions(+), 6 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index fd5ca52..3c4be22 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1286,6 +1286,8 @@ def __init__(self):
optparse.make_option("--export-labels", dest="exportLabels", action="store_true"),
optparse.make_option("--dry-run", "-n", dest="dry_run", action="store_true"),
optparse.make_option("--prepare-p4-only", dest="prepare_p4_only", action="store_true"),
+ optparse.make_option("--shelve-only", dest="shelve_only", action="store_true", help="Create P4 shelf for first change that would be submitted (using a new CL)"),
+ optparse.make_option("--shelve-cl", dest="shelve_cl", help="Replace shelf under existing CL number (previously shelved files will be deleted)"),
optparse.make_option("--conflict", dest="conflict_behavior",
choices=self.conflict_behavior_choices),
optparse.make_option("--branch", dest="branch"),
@@ -1297,6 +1299,8 @@ def __init__(self):
self.preserveUser = gitConfigBool("git-p4.preserveUser")
self.dry_run = False
self.prepare_p4_only = False
+ self.shelve_only = False
+ self.shelve_cl = None
self.conflict_behavior = None
self.isWindows = (platform.system() == "Windows")
self.exportLabels = False
@@ -1496,6 +1500,12 @@ def prepareSubmitTemplate(self):
else:
inFilesSection = False
else:
+ if self.shelve_only and self.shelve_cl:
+ if line.startswith("Change:"):
+ line = "Change: %s\n" % self.shelve_cl
+ if line.startswith("Status:"):
+ line = "Status: pending\n"
+
if line.startswith("Files:"):
inFilesSection = True
@@ -1785,7 +1795,11 @@ def applyCommit(self, id):
if self.isWindows:
message = message.replace("\r\n", "\n")
submitTemplate = message[:message.index(separatorLine)]
- p4_write_pipe(['submit', '-i'], submitTemplate)
+
+ if self.shelve_only:
+ p4_write_pipe(['shelve', '-i', '-r'], submitTemplate)
+ else:
+ p4_write_pipe(['submit', '-i'], submitTemplate)
if self.preserveUser:
if p4User:
@@ -1799,12 +1813,17 @@ def applyCommit(self, id):
# new file. This leaves it writable, which confuses p4.
for f in pureRenameCopy:
p4_sync(f, "-f")
- submitted = True
+
+ if not self.shelve_only:
+ submitted = True
finally:
# skip this patch
if not submitted:
- print "Submission cancelled, undoing p4 changes."
+ if not self.shelve_only:
+ print "Submission cancelled, undoing p4 changes."
+ else:
+ print "Change shelved, undoing p4 changes."
for f in editedFiles:
p4_revert(f)
for f in filesToAdd:
@@ -2034,9 +2053,13 @@ def run(self, args):
if ok:
applied.append(commit)
else:
- if self.prepare_p4_only and i < last:
- print "Processing only the first commit due to option" \
- " --prepare-p4-only"
+ if (self.prepare_p4_only or self.shelve_only) and i < last:
+ if self.prepare_p4_only:
+ print "Processing only the first commit due to option" \
+ " --prepare-p4-only"
+ else:
+ print "Processing only the first commit due to option" \
+ " --shelve-only"
break
if i < last:
quit = False
@@ -3638,6 +3661,7 @@ def printUsage(commands):
"debug" : P4Debug,
"submit" : P4Submit,
"commit" : P4Submit,
+ "shelve" : P4Submit,
"sync" : P4Sync,
"rebase" : P4Rebase,
"clone" : P4Clone,
--
https://github.com/git/git/pull/309
^ permalink raw reply related
* Re: git 2.11.0 error when pushing to remote located on a windows share
From: Torsten Bögershausen @ 2016-12-06 3:09 UTC (permalink / raw)
To: thomas.attwood, peff; +Cc: git
In-Reply-To: <AABB04BF1441D24CB4E9FCF46394F17D666F3805@exchmbx01>
On 2016-12-05 12:05, thomas.attwood@stfc.ac.uk wrote:
> On Sun, 4 Dec 2016 08:09:14 +0000, Torsten Bögershausen wrote:
>> There seems to be another issue, which may or may not being related:
>> https://github.com/git-for-windows/git/issues/979
>
> I think this is the same issue. I've posted my trace command output there as
> It might be more appropriate:
> https://github.com/git-for-windows/git/issues/979#issuecomment-264816175
>
Thanks for the trace.
I think that the problem comes from the "cwd", when a UNC name is used.
cd //SERVER/share/somedir
does not work under Windows, the is no chance to change into that directory.
Does anybody know out of his head why and since when we change the directory
like this ?
Or "git bisect" may help.
^ permalink raw reply
* Re: [PATCH] git-p4: add p4 shelf support
From: Luke Diamand @ 2016-12-06 8:36 UTC (permalink / raw)
To: Nuno Subtil; +Cc: Git Users, Junio C Hamano, Vinicius Kursancew, Lars Schneider
In-Reply-To: <01020158d1de0e71-ac079bb9-bc7d-4fb7-9ff7-60fd6955116b-000000@eu-west-1.amazonses.com>
On 6 December 2016 at 02:02, Nuno Subtil <subtil@gmail.com> wrote:
> Extends the submit command to support shelving a commit instead of
> submitting it to p4 (similar to --prepare-p4-only).
Is this just the same as these two changes?
http://www.spinics.net/lists/git/msg290755.html
http://www.spinics.net/lists/git/msg291103.html
Thanks,
Luke
>
> Signed-off-by: Nuno Subtil <subtil@gmail.com>
> ---
> git-p4.py | 36 ++++++++++++++++++++++++++++++------
> 1 file changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index fd5ca52..3c4be22 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1286,6 +1286,8 @@ def __init__(self):
> optparse.make_option("--export-labels", dest="exportLabels", action="store_true"),
> optparse.make_option("--dry-run", "-n", dest="dry_run", action="store_true"),
> optparse.make_option("--prepare-p4-only", dest="prepare_p4_only", action="store_true"),
> + optparse.make_option("--shelve-only", dest="shelve_only", action="store_true", help="Create P4 shelf for first change that would be submitted (using a new CL)"),
> + optparse.make_option("--shelve-cl", dest="shelve_cl", help="Replace shelf under existing CL number (previously shelved files will be deleted)"),
> optparse.make_option("--conflict", dest="conflict_behavior",
> choices=self.conflict_behavior_choices),
> optparse.make_option("--branch", dest="branch"),
> @@ -1297,6 +1299,8 @@ def __init__(self):
> self.preserveUser = gitConfigBool("git-p4.preserveUser")
> self.dry_run = False
> self.prepare_p4_only = False
> + self.shelve_only = False
> + self.shelve_cl = None
> self.conflict_behavior = None
> self.isWindows = (platform.system() == "Windows")
> self.exportLabels = False
> @@ -1496,6 +1500,12 @@ def prepareSubmitTemplate(self):
> else:
> inFilesSection = False
> else:
> + if self.shelve_only and self.shelve_cl:
> + if line.startswith("Change:"):
> + line = "Change: %s\n" % self.shelve_cl
> + if line.startswith("Status:"):
> + line = "Status: pending\n"
> +
> if line.startswith("Files:"):
> inFilesSection = True
>
> @@ -1785,7 +1795,11 @@ def applyCommit(self, id):
> if self.isWindows:
> message = message.replace("\r\n", "\n")
> submitTemplate = message[:message.index(separatorLine)]
> - p4_write_pipe(['submit', '-i'], submitTemplate)
> +
> + if self.shelve_only:
> + p4_write_pipe(['shelve', '-i', '-r'], submitTemplate)
> + else:
> + p4_write_pipe(['submit', '-i'], submitTemplate)
>
> if self.preserveUser:
> if p4User:
> @@ -1799,12 +1813,17 @@ def applyCommit(self, id):
> # new file. This leaves it writable, which confuses p4.
> for f in pureRenameCopy:
> p4_sync(f, "-f")
> - submitted = True
> +
> + if not self.shelve_only:
> + submitted = True
>
> finally:
> # skip this patch
> if not submitted:
> - print "Submission cancelled, undoing p4 changes."
> + if not self.shelve_only:
> + print "Submission cancelled, undoing p4 changes."
> + else:
> + print "Change shelved, undoing p4 changes."
> for f in editedFiles:
> p4_revert(f)
> for f in filesToAdd:
> @@ -2034,9 +2053,13 @@ def run(self, args):
> if ok:
> applied.append(commit)
> else:
> - if self.prepare_p4_only and i < last:
> - print "Processing only the first commit due to option" \
> - " --prepare-p4-only"
> + if (self.prepare_p4_only or self.shelve_only) and i < last:
> + if self.prepare_p4_only:
> + print "Processing only the first commit due to option" \
> + " --prepare-p4-only"
> + else:
> + print "Processing only the first commit due to option" \
> + " --shelve-only"
> break
> if i < last:
> quit = False
> @@ -3638,6 +3661,7 @@ def printUsage(commands):
> "debug" : P4Debug,
> "submit" : P4Submit,
> "commit" : P4Submit,
> + "shelve" : P4Submit,
> "sync" : P4Sync,
> "rebase" : P4Rebase,
> "clone" : P4Clone,
>
> --
> https://github.com/git/git/pull/309
^ permalink raw reply
* Re: [PATCH] clone,fetch: explain the shallow-clone option a little more clearly
From: Duy Nguyen @ 2016-12-06 9:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Alex Henrie, Git Mailing List
In-Reply-To: <xmqq37i25iy5.fsf@gitster.mtv.corp.google.com>
On Tue, Dec 6, 2016 at 1:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I however offhand do not think the feature can be used to make the
> repository shallower
I'm pretty sure it can, though it's a waste because you should be able
to shorten your history without talking to a remote server. But that
no-remote shortening is not implemented yet. And you probably want an
option to check with remote anyway to make sure the part you cut out
is available there, no history will be lost.
--
Duy
^ permalink raw reply
* Re: [PATCH] commit: make --only --allow-empty work without paths
From: Andreas Krey @ 2016-12-06 9:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqqh96i3ygs.fsf@gitster.mtv.corp.google.com>
On Mon, 05 Dec 2016 12:36:19 +0000, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> > On Sat, Dec 03, 2016 at 07:59:49AM +0100, Andreas Krey wrote:
...
> >> Tool: git commit --allow-empty -m 'FIX: A-123'
> >
> > OK. I think "tool" is slightly funny here, but I get that is part of the
> > real world works. Thanks for illustrating.
>
> I am not sure if I understand. Why isn't the FIX: thing added to
> the commit being pulled by amending it?
Because we don't allow push -f on our blessed repo (bitbucket).
(Oops, answer to wrong question. But the integrators don't want
to meddle with dev's commits, either.)
This has multiple reasons:
- The percentage of people who can and would be willing
to do rebase -i is small. (Not that they are likely to
increase under this policy.)
- Our build tool record builds by commit id, and when
you rebase (even if only for commit message edits)
you lose your (simple) build history.
> Would the convention be for
> the responder of a pull-request to fetch and drop the tip commit?
No, they need to keep it as there is automation hinging on the FIX line.
I would much prefer people to do rebases/amends instead of this crutch,
but that's not for now.
Hmm, it just occurred to me that we might allow force pushes for specific
users to keep the foot-shooting ratio low.
- Andreas
--
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800
^ permalink raw reply
* [PATCH v2 1/6] shallow.c: rename fields in paint_info to better express their purposes
From: Nguyễn Thái Ngọc Duy @ 2016-12-06 12:53 UTC (permalink / raw)
To: git; +Cc: rv, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161206125339.16803-1-pclouds@gmail.com>
paint_alloc() is basically malloc(), tuned for allocating a fixed number
of bits on every call without worrying about freeing any individual
allocation since all will be freed at the end. It does it by allocating
a big block of memory every time it runs out of "free memory". "slab" is
a poor choice of name, at least poorer than "pool".
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
shallow.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/shallow.c b/shallow.c
index 4d0b005..8100dfd 100644
--- a/shallow.c
+++ b/shallow.c
@@ -434,9 +434,9 @@ define_commit_slab(ref_bitmap, uint32_t *);
struct paint_info {
struct ref_bitmap ref_bitmap;
unsigned nr_bits;
- char **slab;
+ char **pools;
char *free, *end;
- unsigned slab_count;
+ unsigned pool_count;
};
static uint32_t *paint_alloc(struct paint_info *info)
@@ -444,11 +444,11 @@ static uint32_t *paint_alloc(struct paint_info *info)
unsigned nr = (info->nr_bits + 31) / 32;
unsigned size = nr * sizeof(uint32_t);
void *p;
- if (!info->slab_count || info->free + size > info->end) {
- info->slab_count++;
- REALLOC_ARRAY(info->slab, info->slab_count);
+ if (!info->pool_count || info->free + size > info->end) {
+ info->pool_count++;
+ REALLOC_ARRAY(info->pools, info->pool_count);
info->free = xmalloc(COMMIT_SLAB_SIZE);
- info->slab[info->slab_count - 1] = info->free;
+ info->pools[info->pool_count - 1] = info->free;
info->end = info->free + COMMIT_SLAB_SIZE;
}
p = info->free;
@@ -624,9 +624,9 @@ void assign_shallow_commits_to_refs(struct shallow_info *info,
post_assign_shallow(info, &pi.ref_bitmap, ref_status);
clear_ref_bitmap(&pi.ref_bitmap);
- for (i = 0; i < pi.slab_count; i++)
- free(pi.slab[i]);
- free(pi.slab);
+ for (i = 0; i < pi.pool_count; i++)
+ free(pi.pools[i]);
+ free(pi.pools);
free(shallow);
}
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 5/6] shallow.c: bit manipulation tweaks
From: Nguyễn Thái Ngọc Duy @ 2016-12-06 12:53 UTC (permalink / raw)
To: git; +Cc: rv, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161206125339.16803-1-pclouds@gmail.com>
From: Rasmus Villemoes <rv@rasmusvillemoes.dk>
First of all, 1 << 31 is technically undefined behaviour, so let's just
use an unsigned literal.
If i is 'signed int' and gcc doesn't know that i is positive, gcc
generates code to compute the C99-mandated values of "i / 32" and "i %
32", which is a lot more complicated than simple a simple shifts/mask.
The only caller of paint_down actually passes an "unsigned int" value,
but the prototype of paint_down causes (completely well-defined)
conversion to signed int, and gcc has no way of knowing that the
converted value is non-negative. Just make the id parameter unsigned.
In update_refstatus, the change in generated code is much smaller,
presumably because gcc is smart enough to see that i starts as 0 and is
only incremented, so it is allowed (per the UD of signed overflow) to
assume that i is always non-negative. But let's just help less smart
compilers generate good code anyway.
Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
shallow.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/shallow.c b/shallow.c
index 719f699..beb967e 100644
--- a/shallow.c
+++ b/shallow.c
@@ -467,7 +467,7 @@ static uint32_t *paint_alloc(struct paint_info *info)
* all walked commits.
*/
static void paint_down(struct paint_info *info, const unsigned char *sha1,
- int id)
+ unsigned int id)
{
unsigned int i, nr;
struct commit_list *head = NULL;
@@ -479,7 +479,7 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
if (!c)
return;
memset(bitmap, 0, bitmap_size);
- bitmap[id / 32] |= (1 << (id % 32));
+ bitmap[id / 32] |= (1U << (id % 32));
commit_list_insert(c, &head);
while (head) {
struct commit_list *p;
@@ -653,11 +653,11 @@ static int add_ref(const char *refname, const struct object_id *oid,
static void update_refstatus(int *ref_status, int nr, uint32_t *bitmap)
{
- int i;
+ unsigned int i;
if (!ref_status)
return;
for (i = 0; i < nr; i++)
- if (bitmap[i / 32] & (1 << (i % 32)))
+ if (bitmap[i / 32] & (1U << (i % 32)))
ref_status[i]++;
}
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 4/6] shallow.c: avoid theoretical pointer wrap-around
From: Nguyễn Thái Ngọc Duy @ 2016-12-06 12:53 UTC (permalink / raw)
To: git; +Cc: rv, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161206125339.16803-1-pclouds@gmail.com>
From: Rasmus Villemoes <rv@rasmusvillemoes.dk>
The expression info->free+size is technically undefined behaviour in
exactly the case we want to test for. Moreover, the compiler is likely
to translate the expression to
(unsigned long)info->free + size > (unsigned long)info->end
where there's at least a theoretical chance that the LHS could wrap
around 0, giving a false negative.
This might as well be written using pointer subtraction avoiding these
issues.
Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
shallow.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/shallow.c b/shallow.c
index 75e1702..719f699 100644
--- a/shallow.c
+++ b/shallow.c
@@ -446,7 +446,7 @@ static uint32_t *paint_alloc(struct paint_info *info)
unsigned nr = (info->nr_bits + 31) / 32;
unsigned size = nr * sizeof(uint32_t);
void *p;
- if (!info->pool_count || info->free + size > info->end) {
+ if (!info->pool_count || size > info->end - info->free) {
if (size > POOL_SIZE)
die("BUG: pool size too small for %d in paint_alloc()",
size);
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 6/6] shallow.c: remove useless code
From: Nguyễn Thái Ngọc Duy @ 2016-12-06 12:53 UTC (permalink / raw)
To: git; +Cc: rv, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161206125339.16803-1-pclouds@gmail.com>
Some context before we talk about the removed code.
This paint_down() is part of step 6 of 58babff (shallow.c: the 8 steps
to select new commits for .git/shallow - 2013-12-05). When we fetch from
a shallow repository, we need to know if one of the new/updated refs
needs new "shallow commits" in .git/shallow (because we don't have
enough history of those refs) and which one.
The question at step 6 is, what (new) shallow commits are required in
other to maintain reachability throughout the repository _without_
cutting our history short? To answer, we mark all commits reachable from
existing refs with UNINTERESTING ("rev-list --not --all"), mark shallow
commits with BOTTOM, then for each new/updated refs, walk through the
commit graph until we either hit UNINTERESTING or BOTTOM, marking the
ref on the commit as we walk.
After all the walking is done, we check the new shallow commits. If we
have not seen any new ref marked on a new shallow commit, we know all
new/updated refs are reachable using just our history and .git/shallow.
The shallow commit in question is not needed and can be thrown away.
So, the code.
The loop here (to walk through commits) is basically
1. get one commit from the queue
2. ignore if it's SEEN or UNINTERESTING
3. mark it
4. go through all the parents and..
5a. mark it if it's never marked before
5b. put it back in the queue
What we do in this patch is drop step 5a because it is not
necessary. The commit being marked at 5a is put back on the queue, and
will be marked at step 3 at the next iteration. The only case it will
not be marked is when the commit is already marked UNINTERESTING (5a
does not check this), which will be ignored at step 2.
But we don't care about refs marking on UNINTERESTING. We care about the
marking on _shallow commits_ that are not reachable from our current
history (and having UNINTERESTING on it means it's reachable). So it's
ok for an UNINTERESTING not to be ref-marked.
Reported-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
shallow.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/shallow.c b/shallow.c
index beb967e..11f7dde 100644
--- a/shallow.c
+++ b/shallow.c
@@ -512,12 +512,8 @@ static void paint_down(struct paint_info *info, const unsigned char *sha1,
oid_to_hex(&c->object.oid));
for (p = c->parents; p; p = p->next) {
- uint32_t **p_refs = ref_bitmap_at(&info->ref_bitmap,
- p->item);
if (p->item->object.flags & SEEN)
continue;
- if (*p_refs == NULL || *p_refs == *refs)
- *p_refs = *refs;
commit_list_insert(p->item, &head);
}
}
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 3/6] shallow.c: make paint_alloc slightly more robust
From: Nguyễn Thái Ngọc Duy @ 2016-12-06 12:53 UTC (permalink / raw)
To: git; +Cc: rv, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161206125339.16803-1-pclouds@gmail.com>
paint_alloc() allocates a big block of memory and splits it into
smaller, fixed size, chunks of memory whenever it's called. Each chunk
contains enough bits to present all "new refs" [1] in a fetch from a
shallow repository.
We do not check if the new "big block" is smaller than the requested
memory chunk though. If it happens, we'll happily pass back a memory
region smaller than expected. Which will lead to problems eventually.
A normal fetch may add/update a dozen new refs. Let's stay on the
"reasonably extreme" side and say we need 16k refs (or bits from
paint_alloc's perspective). Each chunk of memory would be 2k, much
smaller than the memory pool (512k).
So, normally, the under-allocation situation should never happen. A bad
guy, however, could make a fetch that adds more than 4m new/updated refs
to this code which results in a memory chunk larger than pool size.
Check this case and abort.
Noticed-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
[1] Details are in commit message of 58babff (shallow.c: the 8 steps to
select new commits for .git/shallow - 2013-12-05), step 6.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
shallow.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/shallow.c b/shallow.c
index 2512ed3..75e1702 100644
--- a/shallow.c
+++ b/shallow.c
@@ -447,6 +447,9 @@ static uint32_t *paint_alloc(struct paint_info *info)
unsigned size = nr * sizeof(uint32_t);
void *p;
if (!info->pool_count || info->free + size > info->end) {
+ if (size > POOL_SIZE)
+ die("BUG: pool size too small for %d in paint_alloc()",
+ size);
info->pool_count++;
REALLOC_ARRAY(info->pools, info->pool_count);
info->free = xmalloc(POOL_SIZE);
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* [PATCH v2 2/6] shallow.c: stop abusing COMMIT_SLAB_SIZE for paint_info's memory pools
From: Nguyễn Thái Ngọc Duy @ 2016-12-06 12:53 UTC (permalink / raw)
To: git; +Cc: rv, Jeff King, Nguyễn Thái Ngọc Duy
In-Reply-To: <20161206125339.16803-1-pclouds@gmail.com>
We need to allocate a "big" block of memory in paint_alloc(). The exact
size does not really matter. But the pool size has no relation with
commit-slab. Stop using that macro here.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
shallow.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/shallow.c b/shallow.c
index 8100dfd..2512ed3 100644
--- a/shallow.c
+++ b/shallow.c
@@ -431,6 +431,8 @@ void remove_nonexistent_theirs_shallow(struct shallow_info *info)
define_commit_slab(ref_bitmap, uint32_t *);
+#define POOL_SIZE (512 * 1024)
+
struct paint_info {
struct ref_bitmap ref_bitmap;
unsigned nr_bits;
@@ -447,9 +449,9 @@ static uint32_t *paint_alloc(struct paint_info *info)
if (!info->pool_count || info->free + size > info->end) {
info->pool_count++;
REALLOC_ARRAY(info->pools, info->pool_count);
- info->free = xmalloc(COMMIT_SLAB_SIZE);
+ info->free = xmalloc(POOL_SIZE);
info->pools[info->pool_count - 1] = info->free;
- info->end = info->free + COMMIT_SLAB_SIZE;
+ info->end = info->free + POOL_SIZE;
}
p = info->free;
info->free += size;
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Johannes Schindelin @ 2016-12-06 13:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <xmqqy3zu43yk.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 5 Dec 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Seriously, do you really think it is a good idea to have
> > git_config_get_value() *ignore* any value in .git/config?
>
> When we do not know ".git/" directory we see is the repository we were
> told to work on, then we should ignore ".git/config" file. So allowing
> git_config_get_value() to _ignore_ ".git/config" before the program
> calls setup_git_directory() does have its uses.
I think you are wrong. This is yet another inconsistent behavior that
violates the Law of Least Surprises.
> > We need to fix this.
>
> I have a feeling that "environment modifications that cannot be undone"
> we used as the rationale in 73c2779f42 ("builtin-am: implement skeletal
> builtin am", 2015-08-04) might be overly pessimistic and in order to
> implement undo_setup_git_directory(), all we need to do may just be
> matter of doing a chdir(2) back to prefix and unsetting GIT_PREFIX
> environment, but I haven't looked into details of the setup sequence
> recently.
The problem is that we may not know enough anymore to "undo
setup_git_directory()", as some environment variables may have been set
before calling Git. If we simply unset the environment variables, we do it
incorrectly. On the other hand, the environment variables *may* have been
set by setup_git_directory(). In which case we *do* have to unset them.
The entire setup_git_directory() logic is a bit of a historically-grown
garden.
Ciao,
Dscho
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox