* Re: [RESEND PATCH] git-gui--askpass: generalize the window title
From: Sebastian Schuberth @ 2017-03-07 18:40 UTC (permalink / raw)
To: Stefan Beller; +Cc: Pat Thoyts, git@vger.kernel.org
In-Reply-To: <CAGZ79ka_5QogUEwF6SPCwyqSrCNSrtAsqzqJQdXsJkZEAyzDNA@mail.gmail.com>
On Tue, Mar 7, 2017 at 7:30 PM, Stefan Beller <sbeller@google.com> wrote:
> Although the following are included in git.git repository, they have their
> own authoritative repository and maintainers:
Thanks. I continuously get confused by this fact.
--
Sebastian Schuberth
^ permalink raw reply
* Re: Reg : GSoC 2017 Microproject
From: Christian Couder @ 2017-03-07 20:33 UTC (permalink / raw)
To: Vedant Bassi; +Cc: git
In-Reply-To: <CACczA6WCdu0sdd31R2Z6xbr=meo5PTtcOVYCdVHdgZXAfK-3rg@mail.gmail.com>
Hi,
On Tue, Mar 7, 2017 at 11:22 AM, Vedant Bassi <sharababy.dev@gmail.com> wrote:
> Hi,
>
> I would like to participate in GSoC 2017 and I have chosen the Use
> unsigned integral type for collection of bits , idea from the Micro
> projects list.
>
> I request the help of the community for clarifying a few questions that I have.
>
> 1. Is this Microproject already taken ?
As already suggested, it is a good idea to search the mailing list.
This way you can find if it has already been taken or discussed.
You can use https://public-inbox.org/git/ for that.
> 2. If it is free , I would like to point out one place where a signed
> int is used .
>
> In bisect.h , the structure struct rev_list_info uses flags of
> type signed int but , the value of MSB is not checked as a test case
> for any error checking. Hence it can be of type unsigned int.
> It is only used in rev-list.c for checking cases (BISECT_SHOW_ALL and
> REV_LIST_QUIET ).
>
> Is this a valid case.
Yeah, it looks like it is a valid case.
^ permalink raw reply
* Re: [PATCH] repack: Add options to preserve and prune old pack files
From: Junio C Hamano @ 2017-03-07 20:33 UTC (permalink / raw)
To: James Melvin; +Cc: git, nasserg, mfick, peff, sbeller
In-Reply-To: <20170307164035.27866-1-jmelvin@codeaurora.org>
James Melvin <jmelvin@codeaurora.org> writes:
> These options are designed to prevent stale file handle exceptions
> during git operations which can happen on users of NFS repos when
> repacking is done on them. The strategy is to preserve old pack files
> around until the next repack with the hopes that they will become
> unreferenced by then and not cause any exceptions to running processes
> when they are finally deleted (pruned).
I find it a very sensible strategy to work around NFS, but it does
not explain why the directory the old ones are moved to need to be
configurable. It feels to me that a boolean that causes the old
ones renamed s/^pack-/^old-&/ in the same directory (instead of
pruning them right away) would risk less chances of mistakes (e.g.
making "preserved" subdirectory on a separate device mounted there
in a hope to reduce disk usage of the primary repository, which
may defeat the whole point of moving the still-active file around
instead of removing them).
And if we make "preserve-old" a boolean, perhaps the presence of
"prune-preserved" would serve as a substitute for it, iow, perhaps
we may only need --prune-preserved option (and repack.prunePreserved
configuration variable)?
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 677bc7c81..f1a0c97f3 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -10,8 +10,10 @@
>
> static int delta_base_offset = 1;
> static int pack_kept_objects = -1;
> +static int preserve_oldpacks = 0;
> +static int prune_preserved = 0;
We avoid initializing statics to 0 or NULL and instead let BSS take
care of them...
> static int write_bitmaps;
> -static char *packdir, *packtmp;
> +static char *packdir, *packtmp, *preservedir;
... just like what you did here.
> @@ -108,6 +110,27 @@ static void get_non_kept_pack_filenames(struct s
> ...
> +static void preserve_pack(const char *file_path, const char *file_name, const char *file_ext)
> +{
> + char *fname_old;
> +
> + if (mkdir(preservedir, 0700) && errno != EEXIST)
> + error(_("failed to create preserve directory"));
You do not want to do the rest of this function after issuing this
error, no? Because ...
> +
> + fname_old = mkpathdup("%s/%s.old-%s", preservedir, file_name, ++file_ext);
> + rename(file_path, fname_old);
... this rename(2) would fail, whose error return you would catch
and act on.
> + free(fname_old);
> +}
> +
> +static void remove_preserved_dir(void) {
> + struct strbuf buf = STRBUF_INIT;
> +
> + strbuf_addstr(&buf, preservedir);
> + remove_dir_recursively(&buf, 0);
This is a wrong helper function to use on files and directories
inside .git/; the function is about removing paths in the working
tree.
> @@ -121,7 +144,10 @@ static void remove_redundant_pack(const char *dir_name, const char *base_name)
> for (i = 0; i < ARRAY_SIZE(exts); i++) {
> strbuf_setlen(&buf, plen);
> strbuf_addstr(&buf, exts[i]);
> - unlink(buf.buf);
> + if (preserve_oldpacks)
> + preserve_pack(buf.buf, base_name, exts[i]);
> + else
> + unlink(buf.buf);
OK.
> @@ -194,6 +220,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> N_("maximum size of each packfile")),
> OPT_BOOL(0, "pack-kept-objects", &pack_kept_objects,
> N_("repack objects in packs marked with .keep")),
> + OPT_BOOL(0, "preserve-oldpacks", &preserve_oldpacks,
> + N_("move old pack files into the preserved subdirectory")),
> + OPT_BOOL(0, "prune-preserved", &prune_preserved,
> + N_("prune old pack files from the preserved subdirectory after repacking")),
> OPT_END()
> };
>
> @@ -217,6 +247,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>
> packdir = mkpathdup("%s/pack", get_object_directory());
> packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid());
> + preservedir = mkpathdup("%s/preserved", packdir);
>
> sigchain_push_common(remove_pack_on_signal);
>
> @@ -404,6 +435,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>
> /* End of pack replacement. */
>
> + if (prune_preserved)
> + remove_preserved_dir();
I am not sure if I understand your design. Your model looks to me
like there are two modes of operation. #1 uses "--preserve-old" and
sends old ones to purgatory instead of removing them and #2 uses
"--prune-preserved" to remove all the ones in the purgatory
immediately. A few things that come to my mind immediately:
* When "--prune-preseved" is used, it removes both ancient ones and
more recent ones indiscriminately. Would it make more sense to
"expire" only the older ones while keeping the more recent ones?
* It appears that the main reason you would want --prune-preserved
in this design is after running with "--preserve-old" number of
times, you want to remove really old ones that have accumulated,
and I would imagine that at that point of time, you are only
interested in repack, but the code structure tells me that this
will force the users to first run a repack before pruning.
I suspect that a design that is simpler to explain to the users may
be to add a command line option "--preserve-pruned=<expiration>" and
a matching configuration variable repack.preservePruned, which
defaults to "immediate" (i.e. no preserving), and
- When the value of preserve_pruned is not "immediate", use
preserve_pack() instead of unlink();
- At the end, find preserved packs that are older than the value in
preserve_pruned and unlink() them.
It also may make sense to add another command line option
"--prune-preserved-packs-only" (without matching configuration
variable) that _ONLY_ does the "find older preserved packs and
unlink them" part, without doing any repack.
^ permalink raw reply
* Re: [RFC PATCH] rev-parse: add --show-superproject-working-tree
From: Stefan Beller @ 2017-03-07 20:40 UTC (permalink / raw)
To: Junio C Hamano
Cc: SZEDER Gábor, Benjamin Fuchs, git@vger.kernel.org,
brian m. carlson, ville.skytta
In-Reply-To: <xmqq8toh7wqu.fsf@gitster.mtv.corp.google.com>
On Tue, Mar 7, 2017 at 10:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> So perhaps your superproject_exists() helper can be eliminated
That is what I had originally, but I assumed a strict helper function
for "existence of the superproject" would be interesting in the future,
e.g. for get_superproject_git_dir, or on its own. There was an attempt
to have the shell prompt indicate if you are in a submodule,
which would not need to know the worktree or git dir of the
superproject, but only its existence.
> instead coded in get_superproject_working_tree() in place to do:
>
> - xgetcwd() to get "/local/repo/super/sub/dir".
Did you mean .../super/dir/sub ?
If not and we run this command from a directory inside the
submodule, the usual prefix mechanics should go to the
root of the submodule, such that the ".." will be just enough
to break out of that submodule repo.
The interesting part is in the superproject, to see if we are
in a directory (and where the root of the superproject is).
> - relative_path() to get "dir".
ok.
> - ask "ls-{tree,files} --full-name HEAD dir" to get "160000"
> and "sub/dir".
"ls-files --stage --full-name" to get
160000 ... dir/sub
>
> - subtract "sub/dir" from the tail of the "/local/repo/super/sub/dir"
> you got from xgetcwd() earlier.
makes sense.
>
> - return the result.
>
> with a failure/unmet expectations (like not finding 160000) from any
> step returning an error, or something like that.
That seems better as we only need to spawn one process.
(This could also mean I am bad at reading our own man pages)
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH] t*: avoid using pipes
From: Johannes Sixt @ 2017-03-07 20:39 UTC (permalink / raw)
To: Stefan Beller, Prathamesh Chavan; +Cc: git@vger.kernel.org, Pranit Bauva
In-Reply-To: <CAGZ79kaYi1OLuOKvbCmDrMCq0fZnO2Ry7JML=Puwmx6TTtEYog@mail.gmail.com>
Am 07.03.2017 um 18:21 schrieb Stefan Beller:
> On Tue, Mar 7, 2017 at 8:10 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:
>> I'm Prathamesh Chavan. As a part of my micropraoject I have been working on
>> "Avoid pipes for git related commands in test suites".
Welcome to the Git community!
> * keep it micro first, e.g. just convert one file,
> send to list, wait for reviewers feedback and incorporate that
> (optional step after having done the full development cycle:
> convert all the other files; each as its own patch)
> * split up this one patch into multiple patches, e.g. one
> file per patch, then send a patch series.
Actually, being a *micro* project, it should stay so. Not doing all of
the changes would leave some tasks for other apprentices to get warm
with our review process.
>> https://github.com/pratham-pc/git/tree/micro-proj
>
> While I did look at that, not everyone here in the git community
> does so. (Also for getting your change in, Junio seems to strongly prefer
> patches on list instead of e.g. fetching and cherry-picking from your
> github)
Thank you, Stefan, for digging out one particularly interesting case.
> When looking at the content, the conversion seems a bit mechanical
> (which is fine for a micro project), such as:
> ...
> - test "$(git show --pretty=format:%s | head -n 1)" = "one"
> + test "$(git show --pretty=format:%s >out && head -n 1 <out)" = "one"
> ...
>
> specifically for the "head" command I don't think it makes a
> difference in correctness whether you pipe the file into the tool
> or give the filename, i.e. "head -n 1 out" would work just as fine.
True, but!
The intent of removing git invocations from the upstream of a pipe is
that a failure exit code is able to stop a && chain.
The example above does not achieve this even after removal of the pipe.
The reason is that exit code of process expansions $(...) is usually
ignored. To meet the intent, further changes are necessary, for example to:
git show --pretty=format:%s >out &&
test "$(head -n 1 out)" = "one"
Side note: There is one exception where the exit code of $(...) is not
ignored: when it occurs in the last variable assignment of a command
that consists only of variable assignments.
-- Hannes
^ permalink raw reply
* Re: [PATCH] t*: avoid using pipes
From: Stefan Beller @ 2017-03-07 20:52 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Prathamesh Chavan, git@vger.kernel.org, Pranit Bauva
In-Reply-To: <3026648b-a26c-bc67-62dc-170217d6c2ca@kdbg.org>
On Tue, Mar 7, 2017 at 12:39 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Welcome to the Git community!
>
> Actually, being a *micro* project, it should stay so. Not doing all of the
> changes would leave some tasks for other apprentices to get warm with our
> review process.
right, so just pick one file.
> Thank you, Stefan, for digging out one particularly interesting case.
>
>> When looking at the content, the conversion seems a bit mechanical
>> (which is fine for a micro project), such as:
>> ...
>> - test "$(git show --pretty=format:%s | head -n 1)" = "one"
>> + test "$(git show --pretty=format:%s >out && head -n 1 <out)" = "one"
>> ...
>>
>> specifically for the "head" command I don't think it makes a
>> difference in correctness whether you pipe the file into the tool
>> or give the filename, i.e. "head -n 1 out" would work just as fine.
>
>
> True, but!
>
> The intent of removing git invocations from the upstream of a pipe is that a
> failure exit code is able to stop a && chain.
Doh! I was so fixated over discussing whether to use "<" or not,
to miss looking for the actual goal.
Thanks for spotting!
Stefan
^ permalink raw reply
* Crash on MSYS2 with GIT_WORK_TREE
From: valtron @ 2017-03-07 21:28 UTC (permalink / raw)
To: git
Git 1.12.0. When GIT_WORK_TREE contains a drive-letter and
forward-slashes, some git commands crash:
C:\repo>set GIT_WORK_TREE=C:/repo
C:\repo>git rev-parse HEAD
1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping
stack trace to git.exe.stackdump
C:\repo>set GIT_WORK_TREE=
C:\repo>git rev-parse HEAD
a394e40861e1012a96f9578a1f0cf0c5a49ede11
On the other hand, "C:\repo" and "/c/repo" don't have this issue.
Stacktrace from GDB (on git-rev-parse) is:
#0 0x000000018019634d in strcmp (s1=0x600062080 "/c/repo", s2=0x0)
at /msys_scripts/msys2-runtime/src/msys2-runtime/newlib/libc/string/strcmp.c:83
#1 0x00000001005239f1 in ?? ()
#2 0x0000000100523f36 in ?? ()
#3 0x000000010046c6fa in ?? ()
#4 0x0000000100401b6d in ?? ()
#5 0x0000000100401e4b in ?? ()
#6 0x0000000100563593 in ?? ()
#7 0x0000000180047c37 in _cygwin_exit_return ()
at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/dcrt0.cc:1031
#8 0x0000000180045873 in _cygtls::call2 (this=0xffffce00,
func=0x180046bd0 <dll_crt0_1(void*)>, arg=0x0,
buf=buf@entry=0xffffcdf0) at
/msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:40
#9 0x0000000180045924 in _cygtls::call (func=<optimized out>,
arg=<optimized out>)
at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:27
#10 0x0000000000000000 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
It seems "C:/repo" was changed to "/c/repo", but it crashes because it
gets compared to a nullptr.
^ permalink raw reply
* [PULL] git svn branch authentication fix
From: Eric Wong @ 2017-03-07 21:32 UTC (permalink / raw)
To: Junio C Hamano, Hiroshi Shirosaki; +Cc: git
In-Reply-To: <1488779947-25264-1-git-send-email-h.shirosaki@gmail.com>
Thank you. I fixed spelling in the title (s/authenticaton/authentication/),
added my S-o-b, and pushed for Junio to pick up
The following changes since commit 3bc53220cb2dcf709f7a027a3f526befd021d858:
First batch after 2.12 (2017-02-27 14:04:24 -0800)
are available in the git repository at:
git://bogomips.org/git-svn.git svn-auth-branch
for you to fetch changes up to e0688e9b28f2c5ff711460ee8b62077be5df2360:
git svn: fix authentication with 'branch' (2017-03-07 21:29:03 +0000)
----------------------------------------------------------------
Hiroshi Shirosaki (1):
git svn: fix authentication with 'branch'
git-svn.perl | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
^ permalink raw reply
* Re: [PATCH 15/18] read-cache, remove_marked_cache_entries: wipe selected submodules.
From: Junio C Hamano @ 2017-03-07 22:42 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, novalis, sandals, hvoigt, jrnieder, ramsay
In-Reply-To: <20170306205919.9713-16-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> read-cache.c | 27 +++++++++++++++++++++++++--
> 1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 9054369dd0..9a2abacf7a 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -18,6 +18,8 @@
> #include "varint.h"
> #include "split-index.h"
> #include "utf8.h"
> +#include "submodule.h"
> +#include "submodule-config.h"
>
> /* Mask for the name length in ce_flags in the on-disk index */
>
> @@ -520,6 +522,22 @@ int remove_index_entry_at(struct index_state *istate, int pos)
> return 1;
> }
>
> +static void remove_submodule_according_to_strategy(const struct submodule *sub)
> +{
> + switch (sub->update_strategy.type) {
> + case SM_UPDATE_UNSPECIFIED:
> + case SM_UPDATE_CHECKOUT:
> + case SM_UPDATE_REBASE:
> + case SM_UPDATE_MERGE:
> + submodule_move_head(sub->path, "HEAD", NULL, \
What is this backslash doing here?
> + SUBMODULE_MOVE_HEAD_FORCE);
> + break;
> + case SM_UPDATE_NONE:
> + case SM_UPDATE_COMMAND:
> + ; /* Do not touch the submodule. */
> + }
> +}
> +
> /*
> * Remove all cache entries marked for removal, that is where
> * CE_REMOVE is set in ce_flags. This is much more effective than
> @@ -532,8 +550,13 @@ void remove_marked_cache_entries(struct index_state *istate)
>
> for (i = j = 0; i < istate->cache_nr; i++) {
> if (ce_array[i]->ce_flags & CE_REMOVE) {
> - remove_name_hash(istate, ce_array[i]);
> - save_or_free_index_entry(istate, ce_array[i]);
> + const struct submodule *sub = submodule_from_ce(ce_array[i]);
> + if (sub) {
> + remove_submodule_according_to_strategy(sub);
> + } else {
> + remove_name_hash(istate, ce_array[i]);
> + save_or_free_index_entry(istate, ce_array[i]);
> + }
I cannot readily tell as the proposed log message is on the sketchy
side to explain the reasoning behind this, but this behaviour change
is done unconditionally (in other words, without introducing a flag
that is set by --recurse-submodules command line flag and switches
behaviour based on the flag) here. What is the end-user visible
effect with this change? Can we have a new test in this same patch
to demonstrate the desired behaviour introduced by this change (or
the bug this change fixes)?
> }
> else
> ce_array[j++] = ce_array[i];
^ permalink raw reply
* Re: [PATCH 16/18] entry.c: update submodules when interesting
From: Junio C Hamano @ 2017-03-07 22:42 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, novalis, sandals, hvoigt, jrnieder, ramsay
In-Reply-To: <20170306205919.9713-17-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> entry.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/entry.c b/entry.c
> index c6eea240b6..d2b512da90 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -2,6 +2,7 @@
> #include "blob.h"
> #include "dir.h"
> #include "streaming.h"
> +#include "submodule.h"
>
> static void create_directories(const char *path, int path_len,
> const struct checkout *state)
> @@ -146,6 +147,7 @@ static int write_entry(struct cache_entry *ce,
> unsigned long size;
> size_t wrote, newsize = 0;
> struct stat st;
> + const struct submodule *sub;
>
> if (ce_mode_s_ifmt == S_IFREG) {
> struct stream_filter *filter = get_stream_filter(ce->name,
> @@ -203,6 +205,10 @@ static int write_entry(struct cache_entry *ce,
> return error("cannot create temporary submodule %s", path);
> if (mkdir(path, 0777) < 0)
> return error("cannot create submodule directory %s", path);
> + sub = submodule_from_ce(ce);
> + if (sub)
> + return submodule_move_head(ce->name,
> + NULL, oid_to_hex(&ce->oid), SUBMODULE_MOVE_HEAD_FORCE);
> break;
> default:
> return error("unknown file mode for %s in index", path);
> @@ -259,7 +265,31 @@ int checkout_entry(struct cache_entry *ce,
> strbuf_add(&path, ce->name, ce_namelen(ce));
>
> if (!check_path(path.buf, path.len, &st, state->base_dir_len)) {
> + const struct submodule *sub;
> unsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE);
> + /*
> + * Needs to be checked before !changed returns early,
> + * as the possibly empty directory was not changed
> + */
> + sub = submodule_from_ce(ce);
> + if (sub) {
> + int err;
> + if (!is_submodule_populated_gently(ce->name, &err)) {
> + struct stat sb;
> + if (lstat(ce->name, &sb))
> + die(_("could not stat file '%s'"), ce->name);
> + if (!(st.st_mode & S_IFDIR))
> + unlink_or_warn(ce->name);
We found that the path ce->name is supposed to be a submodule that
is checked out, we found that the submodule is not yet populated, we
tried to make sure what is on that path, and its failure would cause
us to die(). All that is sensible.
Then we want to make sure the filesystem entity at the path
currently is a directory and otherwise unlink (i.e. we may have an
unrelated file that is not tracked there, or perhaps the user earlier
decided that replacing the submodule with a single file is a good
idea, but then getting rid of the change with "checkout -f" before
doing "git add" on that file). That is also sensible.
But if that unlink fails, shouldn't we die, just like the case where
we cannot tell what is at the path ce->name?
And if that unlink succeeds, instead of having an empty directory,
we start the "move-head" call to switch from NULL to ce->oid without
having any directory. Wouldn't we want to mkdir() here (and remove
mkdir() in submodule_move_head() if there is one---if there isn't
then I do not think this codepath would work)?
If we had a directory here, but if that directory is not empty,
should we proceed? I am assuming (I haven't carefully read
"move-head") that it is OK because the "run an equivalent of
'checkout --detach ce->oid'" done by "move-head" would notice a
possible information loss to overwrite an untracked path (everything
is "untracked" as the head moves from NULL to ce->oid in this case),
and prevent it from happening.
Am I reading the design correctly?
> + return submodule_move_head(ce->name,
> + NULL, oid_to_hex(&ce->oid),
> + SUBMODULE_MOVE_HEAD_FORCE);
> + } else
> + return submodule_move_head(ce->name,
> + "HEAD", oid_to_hex(&ce->oid),
> + SUBMODULE_MOVE_HEAD_FORCE);
> + }
> +
> if (!changed)
> return 0;
> if (!state->force) {
^ permalink raw reply
* Re: [PATCH 05/18] lib-submodule-update.sh: define tests for recursing into submodules
From: Junio C Hamano @ 2017-03-07 22:26 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, novalis, sandals, hvoigt, jrnieder, ramsay
In-Reply-To: <20170306205919.9713-6-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> + # ... and ignored files are ignroed
ignored.
^ permalink raw reply
* Re: [PATCH 18/18] builtin/read-tree: add --recurse-submodules switch
From: Junio C Hamano @ 2017-03-07 22:42 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, novalis, sandals, hvoigt, jrnieder, ramsay
In-Reply-To: <20170306205919.9713-19-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> A new known failure mode is introduced[1], which is actually not
> a failure but a feature in read-tree. Unlike checkout for which
> the recursive submodule tests were originally written, read-tree does
> warn about ignored untracked files that would be overwritten.
> For the sake of keeping the test library for submodules genric, just
generic
^ permalink raw reply
* Re: [PULL] git svn branch authentication fix
From: Junio C Hamano @ 2017-03-07 22:43 UTC (permalink / raw)
To: Eric Wong; +Cc: Hiroshi Shirosaki, git
In-Reply-To: <20170307213217.GA20443@starla>
Eric Wong <e@80x24.org> writes:
> Thank you. I fixed spelling in the title (s/authenticaton/authentication/),
> added my S-o-b, and pushed for Junio to pick up
>
> The following changes since commit 3bc53220cb2dcf709f7a027a3f526befd021d858:
>
> First batch after 2.12 (2017-02-27 14:04:24 -0800)
>
> are available in the git repository at:
>
> git://bogomips.org/git-svn.git svn-auth-branch
>
> for you to fetch changes up to e0688e9b28f2c5ff711460ee8b62077be5df2360:
>
> git svn: fix authentication with 'branch' (2017-03-07 21:29:03 +0000)
>
> ----------------------------------------------------------------
> Hiroshi Shirosaki (1):
> git svn: fix authentication with 'branch'
>
> git-svn.perl | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Thanks, both. Will pull.
^ permalink raw reply
* Re: [PATCH 4/6] send-pack: improve unpack-status error messages
From: Junio C Hamano @ 2017-03-07 22:56 UTC (permalink / raw)
To: Jeff King; +Cc: Horst Schirmeier, git
In-Reply-To: <20170307133736.4lpn7mgme26dqs3m@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> When the remote tells us that the "unpack" step failed, we
> show an error message. However, unless you are familiar with
> the internals of send-pack and receive-pack, it was not
> clear that this represented an error on the remote side.
> Let's re-word to make that more obvious.
>
> Likewise, when we got an unexpected packet from the other
> end, we complained with a vague message but did not actually
> show the packet. Let's fix that.
Both make sense.
> And finally, neither message was marked for translation. The
> message from the remote probably won't be translated, but
> there's no reason we can't do better for the local half.
Hmm, OK.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> send-pack.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/send-pack.c b/send-pack.c
> index 243633da1..83c23aef6 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -134,9 +134,9 @@ static int receive_unpack_status(int in)
> {
> const char *line = packet_read_line(in, NULL);
> if (!skip_prefix(line, "unpack ", &line))
> - return error("did not receive remote status");
> + return error(_("unable to parse remote unpack status: %s"), line);
> if (strcmp(line, "ok"))
> - return error("unpack failed: %s", line);
> + return error(_("remote unpack failed: %s"), line);
> return 0;
> }
^ permalink raw reply
* Re: fatal error when diffing changed symlinks
From: Johannes Schindelin @ 2017-03-07 22:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, Christophe Macabiau
In-Reply-To: <xmqqfuip7y07.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 7 Mar 2017, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> > When viewing a working tree file, oid.hash could be 0{40} and
> >> > read_sha1_file() is not the right function to use to obtain the
> >> > contents.
> >> >
> >> > Both of these two need to pay attention to 0{40}, I think, as the
> >> > user may be running "difftool -R --dir-diff" in which case the
> >> > working tree would appear in the left hand side instead.
> >>
> >> As a side note, I think even outside of 0{40}, this should be checking
> >> the return value of read_sha1_file(). A corrupted repo should die(), not
> >> segfault.
> >
> > I agree. I am on it.
>
> Friendly ping, if only to make sure that we can keep a piece of this
> thread in the more "recent" pile.
>
> If you have other topics you need to perfect, I think it is OK to
> postpone the fix on this topic a bit longer, but I'd hate to ship
> two releases with a known breakage without an attempt to fix it, so
> if you are otherwise occupied, I may encourage others (including me)
> to take a look at this. The new "difftool" also has a reported
> regression somebody else expressed willingness to work on, which is
> sort of blocked by everybody else not knowing the timeline on this
> one. cf. <20170303212836.GB13790@arch-attack.localdomain>
>
> A patch series would be very welcome, but "Please go ahead if
> somebody else has time, and I'll help reviewing" would be also
> good.
Unfortunately, the obvious fix was not enough. My current work in progress
is in the `difftool-null-sha1` branch on https://gihub.com/dscho/git. I
still have a few other things to tend to, but hope to come back to the
difftool before the end of the week.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 16/18] entry.c: update submodules when interesting
From: Junio C Hamano @ 2017-03-07 23:04 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, bmwill, novalis, sandals, hvoigt, jrnieder, ramsay
In-Reply-To: <xmqqtw746758.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
>> + if (!is_submodule_populated_gently(ce->name, &err)) {
>> + struct stat sb;
>> + if (lstat(ce->name, &sb))
>> + die(_("could not stat file '%s'"), ce->name);
>> + if (!(st.st_mode & S_IFDIR))
>> + unlink_or_warn(ce->name);
>
> We found that the path ce->name is supposed to be a submodule that
> is checked out, we found that the submodule is not yet populated, we
> tried to make sure what is on that path, and its failure would cause
> us to die(). All that is sensible.
> ...
> But if that unlink fails, shouldn't we die, just like the case where
> we cannot tell what is at the path ce->name?
>
> And if that unlink succeeds, instead of having an empty directory,
> we start the "move-head" call to switch from NULL to ce->oid without
> having any directory. Wouldn't we want to mkdir() here (and remove
> mkdir() in submodule_move_head() if there is one---if there isn't
> then I do not think this codepath would work)?
In addition to mkdir(), would we also need the .git file that point
into superproject's .git/modules/ too?
^ permalink raw reply
* Re: [PATCH v4 04/10] setup_git_directory_1(): avoid changing global state
From: Junio C Hamano @ 2017-03-07 23:24 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jeff King, Duy Nguyen
In-Reply-To: <2c8ab22700fb40c9e4e9b46f4981b45db7f2dcf2.1488897111.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> + switch (setup_git_directory_gently_1(&dir, &gitdir)) {
> + case GIT_DIR_NONE:
> + prefix = NULL;
> + break;
> + case GIT_DIR_EXPLICIT:
> + prefix = setup_explicit_git_dir(gitdir.buf, &cwd, nongit_ok);
> + break;
> + case GIT_DIR_DISCOVERED:
> + if (dir.len < cwd.len && chdir(dir.buf))
> + die(_("Cannot change to '%s'"), dir.buf);
> + prefix = setup_discovered_git_dir(gitdir.buf, &cwd, dir.len,
> + nongit_ok);
> + break;
> + case GIT_DIR_BARE:
> + if (dir.len < cwd.len && chdir(dir.buf))
> + die(_("Cannot change to '%s'"), dir.buf);
> + prefix = setup_bare_git_dir(&cwd, dir.len, nongit_ok);
> + break;
> + case GIT_DIR_HIT_CEILING:
> + prefix = setup_nongit(cwd.buf, nongit_ok);
> + break;
> + case GIT_DIR_HIT_MOUNT_POINT:
> + if (nongit_ok) {
> + *nongit_ok = 1;
> + return NULL;
> + }
> + die(_("Not a git repository (or any parent up to mount point %s)\n"
> + "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> + dir.buf);
> + default:
> + die("BUG: unhandled setup_git_directory_1() result");
> + }
I _might_ find niggles in other patches (and other parts of this
patch) that enables the above clean implementation, but this
switch() statement speaks of the value of this entire series ;-)
Very nicely done.
^ permalink raw reply
* Re: [PATCH 16/18] entry.c: update submodules when interesting
From: Stefan Beller @ 2017-03-07 23:08 UTC (permalink / raw)
To: Junio C Hamano
Cc: git@vger.kernel.org, Brandon Williams, David Turner,
brian m. carlson, Heiko Voigt, Jonathan Nieder, Ramsay Jones
In-Reply-To: <xmqqy3wg4rko.fsf@gitster.mtv.corp.google.com>
On Tue, Mar 7, 2017 at 3:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> + if (!is_submodule_populated_gently(ce->name, &err)) {
>>> + struct stat sb;
>>> + if (lstat(ce->name, &sb))
>>> + die(_("could not stat file '%s'"), ce->name);
>>> + if (!(st.st_mode & S_IFDIR))
>>> + unlink_or_warn(ce->name);
>>
>> We found that the path ce->name is supposed to be a submodule that
>> is checked out, we found that the submodule is not yet populated, we
>> tried to make sure what is on that path, and its failure would cause
>> us to die(). All that is sensible.
>> ...
>> But if that unlink fails, shouldn't we die, just like the case where
>> we cannot tell what is at the path ce->name?
>>
>> And if that unlink succeeds, instead of having an empty directory,
>> we start the "move-head" call to switch from NULL to ce->oid without
>> having any directory. Wouldn't we want to mkdir() here (and remove
>> mkdir() in submodule_move_head() if there is one---if there isn't
>> then I do not think this codepath would work)?
>
> In addition to mkdir(), would we also need the .git file that point
> into superproject's .git/modules/ too?
The move_head function takes care of it
Both creation as well as deletion are handled in the move_head function,
when either new or old is NULL.
We can drag that out of that function and have it at the appropriate places
manually.
Why do you think it is better design to have mkdir here and not in move_head?
(For me having everything in move_head was easier to understand,
maybe it is not for others)
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v4 04/10] setup_git_directory_1(): avoid changing global state
From: Brandon Williams @ 2017-03-07 23:35 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <2c8ab22700fb40c9e4e9b46f4981b45db7f2dcf2.1488897111.git.johannes.schindelin@gmx.de>
On 03/07, Johannes Schindelin wrote:
> const char *setup_git_directory_gently(int *nongit_ok)
> {
> + struct strbuf cwd = STRBUF_INIT, dir = STRBUF_INIT, gitdir = STRBUF_INIT;
I couldn't see any strbuf_release() calls for these strbufs so there may
be some memory leaking here.
> const char *prefix;
>
> - prefix = setup_git_directory_gently_1(nongit_ok);
> + /*
> + * We may have read an incomplete configuration before
> + * setting-up the git directory. If so, clear the cache so
> + * that the next queries to the configuration reload complete
> + * configuration (including the per-repo config file that we
> + * ignored previously).
> + */
> + git_config_clear();
> +
> + /*
> + * Let's assume that we are in a git repository.
> + * If it turns out later that we are somewhere else, the value will be
> + * updated accordingly.
> + */
> + if (nongit_ok)
> + *nongit_ok = 0;
> +
> + if (strbuf_getcwd(&cwd))
> + die_errno(_("Unable to read current working directory"));
> + strbuf_addbuf(&dir, &cwd);
> +
> + switch (setup_git_directory_gently_1(&dir, &gitdir)) {
> + case GIT_DIR_NONE:
> + prefix = NULL;
> + break;
> + case GIT_DIR_EXPLICIT:
> + prefix = setup_explicit_git_dir(gitdir.buf, &cwd, nongit_ok);
> + break;
> + case GIT_DIR_DISCOVERED:
> + if (dir.len < cwd.len && chdir(dir.buf))
> + die(_("Cannot change to '%s'"), dir.buf);
> + prefix = setup_discovered_git_dir(gitdir.buf, &cwd, dir.len,
> + nongit_ok);
> + break;
> + case GIT_DIR_BARE:
> + if (dir.len < cwd.len && chdir(dir.buf))
> + die(_("Cannot change to '%s'"), dir.buf);
> + prefix = setup_bare_git_dir(&cwd, dir.len, nongit_ok);
> + break;
> + case GIT_DIR_HIT_CEILING:
> + prefix = setup_nongit(cwd.buf, nongit_ok);
> + break;
> + case GIT_DIR_HIT_MOUNT_POINT:
> + if (nongit_ok) {
> + *nongit_ok = 1;
> + return NULL;
> + }
> + die(_("Not a git repository (or any parent up to mount point %s)\n"
> + "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
> + dir.buf);
> + default:
> + die("BUG: unhandled setup_git_directory_1() result");
> + }
> +
> if (prefix)
> setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
> else
> --
> 2.12.0.windows.1.7.g94dafc3b124
>
>
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 15/18] read-cache, remove_marked_cache_entries: wipe selected submodules.
From: Stefan Beller @ 2017-03-07 23:37 UTC (permalink / raw)
To: Junio C Hamano
Cc: git@vger.kernel.org, Brandon Williams, David Turner,
brian m. carlson, Heiko Voigt, Jonathan Nieder, Ramsay Jones
In-Reply-To: <xmqqfuio674n.fsf@gitster.mtv.corp.google.com>
On Tue, Mar 7, 2017 at 2:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> + submodule_move_head(sub->path, "HEAD", NULL, \
>
> What is this backslash doing here?
I am still bad at following coding conventions, apparently.
will remove.
>> @@ -532,8 +550,13 @@ void remove_marked_cache_entries(struct index_state *istate)
>>
>> for (i = j = 0; i < istate->cache_nr; i++) {
>> if (ce_array[i]->ce_flags & CE_REMOVE) {
>> - remove_name_hash(istate, ce_array[i]);
>> - save_or_free_index_entry(istate, ce_array[i]);
>> + const struct submodule *sub = submodule_from_ce(ce_array[i]);
>> + if (sub) {
>> + remove_submodule_according_to_strategy(sub);
>> + } else {
>> + remove_name_hash(istate, ce_array[i]);
>> + save_or_free_index_entry(istate, ce_array[i]);
>> + }
>
> I cannot readily tell as the proposed log message is on the sketchy
> side to explain the reasoning behind this, but this behaviour change
> is done unconditionally (in other words, without introducing a flag
> that is set by --recurse-submodules command line flag and switches
> behaviour based on the flag) here. What is the end-user visible
> effect with this change?
submodule_from_ce returns always NULL, when such flag is not given.
From 10/18:
+const struct submodule *submodule_from_ce(const struct cache_entry *ce)
+{
+ if (!S_ISGITLINK(ce->ce_mode))
+ return NULL;
+
+ if (!should_update_submodules())
+ return NULL;
+
+ return submodule_from_path(null_sha1, ce->name);
+}
should_update_submodules is always false if such a flag is not set,
such that we end up in the else case which is literally the same as
the removed lines (they are just indented).
> Can we have a new test in this same patch
> to demonstrate the desired behaviour introduced by this change (or
> the bug this change fixes)?
This doesn't fix a bug, but in case the flag is given (in patches 17,18)
this new code removes submodules that are no longer recorded in
the tree.
^ permalink raw reply
* Re: GSoC 2017
From: Valery Tolstov @ 2017-03-07 23:22 UTC (permalink / raw)
To: t.gummerer; +Cc: git, sbeller
Decided to rewrite module_clone to use connect_work_tree_and_git_dir
as my microproject by suggestion of Stefan Bellar.
https://public-inbox.org/git/CAGZ79kY+1E-wg0-uzGJmE+haOE+1WCmg0Eux7rWGtkU_aBDQ9g@mail.gmail.com/
It seems that connect_work_tree_and_git_dir needs to be slightly changed
to deal with submodules. Because path to config file for submodules lies
not directly in .git directory we need to know whether the path is
submodule or not. Can I use submodule_from_path to determine this?
I mean what if NULL result also indicates error sometimes?
Another possible solution is to pass the flag indicating
that path is submodule or not. But I think this solution is bad.
Regards,
Valery Tolstov
^ permalink raw reply
* Re: Crash on MSYS2 with GIT_WORK_TREE
From: Johannes Schindelin @ 2017-03-07 23:03 UTC (permalink / raw)
To: valtron; +Cc: git
In-Reply-To: <CAFKRc7y_kpCGNORENUZ2qw_4qBwjjyaaDFxAEQa52fTryj+w7A@mail.gmail.com>
Hi valtron,
On Tue, 7 Mar 2017, valtron wrote:
> Git 1.12.0.
I take it you meant 2.12.0. And you probably also meant to imply that you
are referring to MSYS2's git.exe in /usr/bin/.
> When GIT_WORK_TREE contains a drive-letter and forward-slashes, some git
> commands crash:
>
> C:\repo>set GIT_WORK_TREE=C:/repo
> C:\repo>git rev-parse HEAD
> 1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping
> stack trace to git.exe.stackdump
This suggests that my assumption above is correct: it looks as if you are
executing <MSYS2>\usr\bin\git.exe here.
The proof lies in the pudding, though, and you are the only person who can
do that unless you post the contents of that git.exe.stackdump.
> Stacktrace from GDB (on git-rev-parse) is:
>
> #0 0x000000018019634d in strcmp (s1=0x600062080 "/c/repo", s2=0x0)
> at /msys_scripts/msys2-runtime/src/msys2-runtime/newlib/libc/string/strcmp.c:83
> #1 0x00000001005239f1 in ?? ()
> #2 0x0000000100523f36 in ?? ()
> #3 0x000000010046c6fa in ?? ()
> #4 0x0000000100401b6d in ?? ()
> #5 0x0000000100401e4b in ?? ()
> #6 0x0000000100563593 in ?? ()
> #7 0x0000000180047c37 in _cygwin_exit_return ()
> at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/dcrt0.cc:1031
> #8 0x0000000180045873 in _cygtls::call2 (this=0xffffce00,
> func=0x180046bd0 <dll_crt0_1(void*)>, arg=0x0,
> buf=buf@entry=0xffffcdf0) at
> /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:40
> #9 0x0000000180045924 in _cygtls::call (func=<optimized out>,
> arg=<optimized out>)
> at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:27
> #10 0x0000000000000000 in ?? ()
> Backtrace stopped: previous frame inner to this frame (corrupt stack?)
This may be the wrong thread, though. You can find out what other threads
there are with `info threads` and switch by `thread <id>`, the
backtrace(s) of the other thread(s) may be informative.
Please also note that installing the `msys2-runtime-devel` package via
Pacman may be able to get you nicer symbols.
In any case, this problem is squarely within the MSYS2 runtime. It has
nothing to do with Git except for the motivation to set an environment
variable to an absolute path as you outlined.
Having said that, the problem also occurs when using *MSYS2* git.exe (i.e.
/usr/bin/git.exe, not /mingw64/bin/git.exe) in the Git for Windows SDK.
Ciao,
Johannes
^ permalink raw reply
* Re: [Request for Documentation] Differentiate signed (commits/tags/pushes)
From: Stefan Beller @ 2017-03-07 22:19 UTC (permalink / raw)
To: Jeff King; +Cc: tom, Matthieu Moy, git@vger.kernel.org, Junio C Hamano
In-Reply-To: <20170307092353.ibirvitsxhzn3apz@sigill.intra.peff.net>
On Tue, Mar 7, 2017 at 1:23 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Mar 06, 2017 at 11:59:24AM -0800, Stefan Beller wrote:
>
>> What is the difference between signed commits and tags?
>> (Not from a technical perspective, but for the end user)
>
> I think git has never really tried to assign any _meaning_ to the
> signatures. It implements the technical bits and leaves it up to the
> user and their workflows to decide what a given signature means.
That is a nihilistic approach? ;)
As a user I would like to know what I can do with such a signed commit.
And I happen to be an experienced user in the sense that I know
about git tag --sign as well as git verify-tag; further reading of
these two man pages tells me I can even use git-tag to verify a
tag. So looking for the verify option in git-commit for signed commits...
well, no. Ah! git-verify-commit it is.
I assumed to have most discussion in git-tag or at least a pointer
from there to further reading.
In an ideal world we might have a manpage git-trust-model(7),
that explains different workflows and when certain signing
mechanisms make sense and what they protect us from.
I might write such a man page (after I get the gitmodules
page done).
Off list I was told
"just look at Documentation/technical/signature-format.txt;
it explains all different things that you can sign or have signed
stuff". But as an end user I refuse to look at that. ;)
>
> People generally seem to take tag signatures to mean one (or both) of:
>
> 1. Certifying that the tree contents at that tag are the official,
> signed release contents (i.e., this is version 1.0, and I'm the
> maintainer, so believe me).
>
> 2. Certifying that all the history leading up to that tag is
> "official" in some sense (i.e., I'm the maintainer, and this was
> the tip of my git tree at the time I tagged the release).
>
> Signing individual commits _could_ convey meaning (2), but "all history
> leading up" part is unlikely to be something that the signer needs to
> enforce on every commit.
I was told signed commits could act as a poor mans
push certificate (again off list :/).
> In my opinion, the most useful meaning for commit-signing is simply to
> cryptographically certify the identity of the committer. We don't
> compare the GPG key ident to the "committer" field, but IMHO that would
> be a reasonable optional feature for verify-commit (I say optional
> because we're starting to assign semantics now).
So the signed commit focuses on the committer instead of the content
(which is what tags are rather used for) ?
> I think one of the reasons kernel (and git) developers aren't that
> interested in signed commits is that they're not really that interesting
> in a patch workflow. You're verifying the committer, who in this project
> is invariably Junio, and we just take his word that whatever is in the
> "author" field is reasonable.
Well in such a workflow Junio could also sign the tip-commits of
pu/next before pushing, such that we can trust it was really him doing
the maintenance work and not his evil twin.
> But for a project whose workflow is based around pushing and pulling
> commits, I think it does make sense. The author field may not always
> match the committer (e.g., in a cherry-pick), but it still lets you
> trace that attestation of the author back to the committer. And it's up
> to UI to make that distinction clear (e.g., if you push a signed
> cherry-pick to GitHub, the commit is labeled with something like "A U
> Thor committed with C O Mitter", and then you get a little "Verified"
> tag for C O Mitter that gives you more information about the signature).
>
> So I don't think it's just a checkbox feature. It's a useful thing for
> certain workflows that really want to be able to attribute individual
> commits with cryptographic strength.
"certain workflows". :(
See, I really like reading e.g. the "On Re-tagging" section of git-tag
as it doesn't hand wave around the decisions to make.
Now as a user I may already have a workflow that I like. And I might
want to "bring in more security". Then I have to figure out possible
attack scenarios and which sort of signing can prevent such an attack.
And each organisation has to do that themselves, but we as the provider
of the tool might have this knowledge because we implemented all
these shiny "sign here, please" parts.
Thanks for the lively discussion,
Stefan
^ permalink raw reply
* Re: [RFC PATCH] rev-parse: add --show-superproject-working-tree
From: Junio C Hamano @ 2017-03-07 22:49 UTC (permalink / raw)
To: Stefan Beller
Cc: SZEDER Gábor, Benjamin Fuchs, git@vger.kernel.org,
brian m. carlson, ville.skytta
In-Reply-To: <CAGZ79kYMZk3sNNjWgp9acQG6z5Q5CnsJi+n7Bvr3EkfbSHasMA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Tue, Mar 7, 2017 at 10:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> So perhaps your superproject_exists() helper can be eliminated
>
> That is what I had originally, but I assumed a strict helper function
> for "existence of the superproject" would be interesting in the future,
> e.g. for get_superproject_git_dir, or on its own. There was an attempt
> to have the shell prompt indicate if you are in a submodule,
> which would not need to know the worktree or git dir of the
> superproject, but only its existence.
>
>> instead coded in get_superproject_working_tree() in place to do:
>>
>> - xgetcwd() to get "/local/repo/super/sub/dir".
>
> Did you mean .../super/dir/sub ?
I meant "/local/repo/super/sub/dir". I am using this case to
illustrate: the root of the superproject is at /local/repo/super,
its submodule we are interested in is at sub/dir, and the function
is working inside the submodule--after the repository discovery
moves the cwd, xgetcwd() would give the root of the working tree of
the submodule, which is at "/local/repo/super/sub/dir".
And that would give us "dir" by taking that as relative to its "../"
>> - relative_path() to get "dir".
>
> ok.
indeed.
>> - ask "ls-{tree,files} --full-name HEAD dir" to get "160000"
>> and "sub/dir".
>
> "ls-files --stage --full-name" to get
> 160000 ... dir/sub
Yeah, also when usihng ls-files you obviously do not give HEAD but
you do give "dir" as the pathspec. And you get "sub/dir" as the
result.
^ permalink raw reply
* Re: [PATCH v4 04/10] setup_git_directory_1(): avoid changing global state
From: Johannes Schindelin @ 2017-03-08 0:57 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <20170307233549.GA128647@google.com>
Hi Brandon,
On Tue, 7 Mar 2017, Brandon Williams wrote:
> On 03/07, Johannes Schindelin wrote:
> > const char *setup_git_directory_gently(int *nongit_ok)
> > {
> > + struct strbuf cwd = STRBUF_INIT, dir = STRBUF_INIT, gitdir = STRBUF_INIT;
>
> I couldn't see any strbuf_release() calls for these strbufs so there may
> be some memory leaking here.
You are correct, of course. Something like this may work:
-- snipsnap --
diff --git a/setup.c b/setup.c
index 9118b48590a..c822582b96e 100644
--- a/setup.c
+++ b/setup.c
@@ -1027,6 +1027,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
case GIT_DIR_HIT_MOUNT_POINT:
if (nongit_ok) {
*nongit_ok = 1;
+ strbuf_release(&cwd);
+ strbuf_release(&dir);
return NULL;
}
die(_("Not a git repository (or any parent up to mount point %s)\n"
@@ -1044,6 +1046,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
startup_info->have_repository = !nongit_ok || !*nongit_ok;
startup_info->prefix = prefix;
+ strbuf_release(&cwd);
+ strbuf_release(&dir);
+ strbuf_release(&gitdir);
+
return prefix;
}
^ permalink raw reply related
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