* 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
* 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
* 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: [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] 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: 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: [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: git init --separate-git-dir does not update symbolic .git links for submodules
From: Stefan Beller @ 2017-03-07 20:11 UTC (permalink / raw)
To: CAGZ79kZbc394rmxYDUxCbysKNbEQCB7aLJkf6MGcCeXKAxiKhA
Cc: git@vger.kernel.org, Junio C Hamano, Valery Tolstov, sven
In-Reply-To: <1488916365.8812.1@smtp.yandex.ru>
On Tue, Mar 7, 2017 at 11:52 AM, Valery Tolstov <me@vtolstov.org> wrote:
> Just noticed that there already is function that gives module list
> module_list_compute. But purpose of it's arguments is not quite clear
> for me at this moment.
>
static int module_list_compute(int argc, const char **argv,
const char *prefix,
struct pathspec *pathspec,
struct module_list *list)
argc, argv and prefix are just passed through from each caller
argc is the number of arguments on the command line,
argv is an array of said arguments on the command line,
prefix is the position inside the repository.
e.g. If in git.git in Documentation/, you run a git command
"git submodule--helper module_list *"
then first "git" and "submodule--helper" are removed,
and then cmd_submodule__helper is called (at the end of
the submodule helper, which then sees
argc=2, argv=["module_list", "*"],
prefix="Documentation/"
which then proceeds to into module_list().
That parses these arguments, but there is no
argument it knows about, so it does nothing.
Then it just passes these three (argc, argv, prefix)
to module_list_compute, which then makes up the list,
to be stored in the last parameter 'list'.
The 'pathspec' parameter seems weird.
Internally the arguments mentioned above are converted to a pathspec,
such that we can go through all files and call match_pathspec inside
of module_list_compute.
In all but one cases we do not care about the pathspec, but in one
case (in update_clone) we want to issue a warning if the pathspec
was empty (i.e. the user just said "git submodule--helper update_clone"
with no further path arguments)
^ permalink raw reply
* Re: RFC: Another proposed hash function transition plan
From: Ian Jackson @ 2017-03-07 18:57 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, sbeller, bmwill, jonathantanmy, peff, Linus Torvalds
In-Reply-To: <20170304011251.GA26789@aiede.mtv.corp.google.com>
Jonathan Nieder writes ("RFC: Another proposed hash function transition plan"):
> This past week we came up with this idea for what a transition to a new
> hash function for Git would look like. I'd be interested in your
> thoughts (especially if you can make them as comments on the document,
> which makes it easier to address them and update the document).
Thanks for this.
This is a reasonable plan. It corresponds to approaches (2) and (B)
of my survey mail from the other day. Ie, two parallel homogeneous
hash trees, rather than a unified but heterogeneous hash tree, with
old vs new object names distinguished by length.
I still prefer my proposal with the mixed hash tree, mostly because
the handling of signatures here is very awkward, and because my
proposal does not involve altering object ids stored other than in the
git object graph (eg CI system databases, etc.)
One thing you've missed, I think, is notes: notes have to be dealt
with in a more complicated way. Do you intend to rewrite the tree
objects for notes commits so that the notes are annotations for the
new names for the annotated objects ? And if so, when ?
Also I think you need to specify how abbreviated object names are
interpreted.
Regards,
Ian.
^ permalink raw reply
* Re: git init --separate-git-dir does not update symbolic .git links for submodules
From: Valery Tolstov @ 2017-03-07 19:52 UTC (permalink / raw)
To: sbeller; +Cc: git, gitster, me, sven
Just noticed that there already is function that gives module list
module_list_compute. But purpose of it's arguments is not quite clear
for me at this moment.
^ permalink raw reply
* Re: git init --separate-git-dir does not update symbolic .git links for submodules
From: Stefan Beller @ 2017-03-07 19:59 UTC (permalink / raw)
To: CAGZ79kZbc394rmxYDUxCbysKNbEQCB7aLJkf6MGcCeXKAxiKhA
Cc: git@vger.kernel.org, Junio C Hamano, Valery Tolstov, sven
In-Reply-To: <1488913150.8812.0@smtp.yandex.ru>
On Tue, Mar 7, 2017 at 10:59 AM, Valery Tolstov <me@vtolstov.org> wrote:
> I think we can reuse code from module_clone that writes .git link.
> Possibly this code fragment needs to be factored out from module_clone
That fragment already exists, see dir.h:
connect_work_tree_and_git_dir(work_tree, git_dir);
Maybe another good microproject is to use that in module_clone.
>
> Also, to fix all the links, we need to obtain the list of submodules
> and then iterate over them.
Right, but a submodule may have a nested submodule.
So we need to fix each submodule from that list recursively,
i.e. not just the submodule itself, but any potential nested
submodule in that submodule, too.
(the listing doesn't list these nested submodules)
So we would call
fix_gitlink(sub)
{
fix_locally(sub);
// have a child process that calls
// this function on any submodule inside sub.
}
> module_list command iterates
> and prints the result to stdout. Maybe we can reuse this output.
> Or create separate function that returns the list of submodules.
yeah you can make use of module_list_compute to just produce
the list internally.
>
> Can we call these functions from submodule--helper inside init command,
> or run them thorugh internal command interface?
Both sounds fine, though calling internally is preferable for
performance reasons.
>
> In my opinion, make submodule--helper fully responsible for link fixes
> would be a good solution. Then we create two additional function, one
> that fixes all submodules on the current level, and another that
> fixes individual submodule.
> Although it looks good, I'm not quite sure that it's really good.
That sounds good to me.
So "git init --separate-git-dir" calls internally a new function
in the submodule--helper to
>
> So, maybe we can do link fixes like this:
>
> 1. Start fixing from init command using submodule--helper, with
> subcommand that fixes all submodules on current level
for this step we do not change the repository we are in, so there
is no need to call a new process, but we rather want to
call it internally.
> 2. Each submodule processed with another subcommand/function in
> submodule--helper individually
sounds good.
> 3. Repeat for current submodule recursively
and this recursive action needs to have its own process in the submodule,
e.g. "git submodule--helper --recursive fix-git-links"
>
> Glad to see your advices.
>
> Regards,
> Valery Tolstov
Regards,
Stefan
^ permalink raw reply
* Re: RFC: Another proposed hash function transition plan
From: Linus Torvalds @ 2017-03-07 19:15 UTC (permalink / raw)
To: Ian Jackson
Cc: Jonathan Nieder, Git Mailing List, Stefan Beller, bmwill,
Jonathan Tan, Jeff King
In-Reply-To: <22719.680.730866.781688@chiark.greenend.org.uk>
On Tue, Mar 7, 2017 at 10:57 AM, Ian Jackson
<ijackson@chiark.greenend.org.uk> wrote:
>
> Also I think you need to specify how abbreviated object names are
> interpreted.
One option might be to not use hex for the new hash, but base64 encoding.
That would make the full size ASCII hash encoding length roughly
similar (43 base64 characters rather than 40), which would offset some
of the new costs (longer filenames in the loose format, for example).
Also, since 256 isn't evenly divisible by 6, and because you'd want
some way to explictly disambiguate the new hashes, the rule *could* be
that the ASCII representation of a new hash is the base64 encoding of
the 258-bit value that has "10" prepended to it as padding.
That way the first character of the hash would be guaranteed to not be
a hex digit, because it would be in the range [g-v] (indexes 32..47).
Of course, the downside is that base64 encoded hashes can also end up
looking very much like real words, and now case would matter too.
The "use base64 with a "10" two-bit padding prepended" also means that
the natural loose format radix format would remain the first 2
characters of the hash, but due to the first character containing the
padding, it would be a fan-out of 2**10 rather than 2**12.
Of course, having written that, I now realize how it would cause
problems for the usual shit-for-brains case-insensitive filesystems.
So I guess base64 encoding doesn't work well for that reason.
Linus
^ permalink raw reply
* Re: git init --separate-git-dir does not update symbolic .git links for submodules
From: Valery Tolstov @ 2017-03-07 18:59 UTC (permalink / raw)
To: sbeller; +Cc: git, gitster, me, sven
I think we can reuse code from module_clone that writes .git link.
Possibly this code fragment needs to be factored out from module_clone
Also, to fix all the links, we need to obtain the list of submodules
and then iterate over them. module_list command iterates
and prints the result to stdout. Maybe we can reuse this output.
Or create separate function that returns the list of submodules.
Can we call these functions from submodule--helper inside init command,
or run them thorugh internal command interface?
In my opinion, make submodule--helper fully responsible for link fixes
would be a good solution. Then we create two additional function, one
that fixes all submodules on the current level, and another that
fixes individual submodule.
Although it looks good, I'm not quite sure that it's really good.
So, maybe we can do link fixes like this:
1. Start fixing from init command using submodule--helper, with
subcommand that fixes all submodules on current level
2. Each submodule processed with another subcommand/function in
submodule--helper individually
3. Repeat for current submodule recursively
Glad to see your advices.
Regards,
Valery Tolstov
^ permalink raw reply
* Re: [PATCH v5 1/1] config: add conditional include
From: Stefan Beller @ 2017-03-07 18:39 UTC (permalink / raw)
To: Jeff King
Cc: Nguyễn Thái Ngọc Duy, git@vger.kernel.org,
Junio C Hamano, Sebastian Schuberth, Matthieu Moy
In-Reply-To: <20170307084717.i2jru77v3rhd443e@sigill.intra.peff.net>
On Tue, Mar 7, 2017 at 12:47 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Mar 06, 2017 at 02:44:27PM -0800, Stefan Beller wrote:
>
>> > +static int include_condition_is_true(const char *cond, size_t cond_len)
>> > +{
>> ...
>> > +
>> > + error(_("unrecognized include condition: %.*s"), (int)cond_len, cond);
>> > + /* unknown conditionals are always false */
>> > + return 0;
>> > +}
>>
>> Thanks for putting an error message here. I was looking at what
>> is currently queued as origin/nd/conditional-config-include,
>> which doesn't have this error() (yet / not any more?)
>
> It's "not any more". It was in the original and I asked for it to be
> removed during the last review.
Okay. The joys of contradicting opinions on a mailing list. :)
>
>> I'd strongly suggest to keep the error message here as that way
>> a user can diagnose e.g. a typo in the condition easily.
>>
>> If we plan to extend this list of conditions in the future, and a user
>> switches between versions of git, then they may see this message
>> on a regular basis (whenever they use the 'old' version).
>
> That would make it unlike the rest of the config-include mechanism
> (which quietly ignores things it doesn't understand, like include.foo,
> or include.foo.path), as well as the config code in general (which
> ignores misspelt keys).
>
> Some of that "quiet when you don't understand it" is historical
> necessity. Older versions _can't_ complain about not knowing
> include.path, because they don't yet know it's worth complaining about.
agreed
> Likewise here, if this ships in v2.13 and a new condition "foo:" ships
> in v2.14, you get:
>
> v2.12 - no complaint; we don't even understand includeIf at all
> v2.13 - complain; we know includeIf, but not "foo:"
> v2.14 - works as expected
>
> Which is kind of weird and inconsistent. But maybe the typo-detection
> case is more important to get right than consistency across historical
> versions.
Oh, I see. I was contemplating a future in which 2.12 is not used anymore.
When looking at other examples, such as url.<...>.insteadOf we also do not
warn about typos (well we can't actually).
In diff.<driver>.(command/binary/..) we know the limited set of drivers,
which is similar to the situation we have here.
Maybe a compromise between typo checking (edit distance < 2 -> warn;
silent for larger distances) and the consistency over time is desired.
But this is even more code to write.
So for now I retract my strong opinion and be happy with what is
presented as-is for the reasons Peff gave.
Thanks,
Stefan
^ permalink raw reply
* Re: [RFC PATCH] rev-parse: add --show-superproject-working-tree
From: Junio C Hamano @ 2017-03-07 18:44 UTC (permalink / raw)
To: Stefan Beller; +Cc: szeder.dev, email, git, sandals, ville.skytta
In-Reply-To: <20170307034553.10770-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> +const char *get_superproject_working_tree()
const char *get_superproject_working_tree(void)
The same for the *.h file declaration.
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf sb = STRBUF_INIT;
> +
> + if (!superproject_exists())
> + return NULL;
> + ...
> + return strbuf_detach(&sb, NULL);
Having reviewed it, I somehow think you do not want to have a
separate superproject_exists() that grabs some part of the
information this caller needs and then discards it.
The helper already does these things:
- xgetcwd(), which may give you "/local/repo/super/sub/dir"
- relative_path() with the result and "..", which may give you
"dir"
- ls-tree HEAD "dir" to see what is in "sub/dir" of the
repository that governs ".."; if "sub/dir" is a gitlink,
you know you started in a working tree of a repository
different from the one that governs "..".
And the caller is trying to figure out where the root of the
superproject is, i.e. "/local/repo/super", and the helper has half
of the answer to that already. If you ask the "ls-tree HEAD" (as I
said, I think it should be "ls-files") to give you not "dir" but
"sub/dir", you can subtract it from the result of xgetcwd() you did
at the beginning of the helper, and that gives what this caller of
the helper wants.
So perhaps your superproject_exists() helper can be eliminated and
instead coded in get_superproject_working_tree() in place to do:
- xgetcwd() to get "/local/repo/super/sub/dir".
- relative_path() to get "dir".
- ask "ls-{tree,files} --full-name HEAD dir" to get "160000"
and "sub/dir".
- subtract "sub/dir" from the tail of the "/local/repo/super/sub/dir"
you got from xgetcwd() earlier.
- return the result.
with a failure/unmet expectations (like not finding 160000) from any
step returning an error, or something like that.
^ permalink raw reply
* [PATCH 3/6] send-pack: use skip_prefix for parsing unpack status
From: Jeff King @ 2017-03-07 13:36 UTC (permalink / raw)
To: Horst Schirmeier; +Cc: git
In-Reply-To: <20170307133437.qee2jtynbiwf6uzr@sigill.intra.peff.net>
This avoids repeating ourselves, and the use of magic
numbers.
Signed-off-by: Jeff King <peff@peff.net>
---
Obviously not necessary, but just a cleanup while I was here.
send-pack.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/send-pack.c b/send-pack.c
index 12e229e44..243633da1 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -133,10 +133,10 @@ static int pack_objects(int fd, struct ref *refs, struct sha1_array *extra, stru
static int receive_unpack_status(int in)
{
const char *line = packet_read_line(in, NULL);
- if (!starts_with(line, "unpack "))
+ if (!skip_prefix(line, "unpack ", &line))
return error("did not receive remote status");
- if (strcmp(line, "unpack ok"))
- return error("unpack failed: %s", line + 7);
+ if (strcmp(line, "ok"))
+ return error("unpack failed: %s", line);
return 0;
}
--
2.12.0.429.gde83c8049
^ permalink raw reply related
* Re: fatal error when diffing changed symlinks
From: Junio C Hamano @ 2017-03-07 18:16 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, git, Christophe Macabiau
In-Reply-To: <alpine.DEB.2.20.1702251336420.3767@virtualbox>
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.
Thanks.
^ permalink raw reply
* Re: [RESEND PATCH] git-gui--askpass: generalize the window title
From: Stefan Beller @ 2017-03-07 18:30 UTC (permalink / raw)
To: Sebastian Schuberth, Pat Thoyts; +Cc: git@vger.kernel.org
In-Reply-To: <0102015aa974d7a6-46afa73b-4378-4b01-9db4-723fb9e41d65-000000@eu-west-1.amazonses.com>
https://public-inbox.org/git/xmqq60jz2xry.fsf@gitster.mtv.corp.google.com/
Although the following are included in git.git repository, they have their
own authoritative repository and maintainers:
- git-gui/ comes from git-gui project, maintained by Pat Thoyts:
git://repo.or.cz/git-gui.git
I cc'd Pat.
Thanks,
Stefan
^ permalink raw reply
* [RESEND PATCH] git-gui--askpass: generalize the window title
From: Sebastian Schuberth @ 2017-03-07 15:48 UTC (permalink / raw)
To: git
In-Reply-To: <000001529cbe5436-285f0113-5761-49d8-8961-5a9df0180ed7-000000@eu-west-1.amazonses.com>
git-gui--askpass is not only used for SSH authentication, but also for
HTTPS. In that context it is confusing to have a window title of
"OpenSSH". So generalize the title so that it also says which parent
process, i.e. Git, requires authentication.
Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
git-gui/git-gui--askpass | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-gui/git-gui--askpass b/git-gui/git-gui--askpass
index 4277f30..1e5c3256 100755
--- a/git-gui/git-gui--askpass
+++ b/git-gui/git-gui--askpass
@@ -60,7 +60,7 @@ proc finish {} {
set ::rc 0
}
-wm title . "OpenSSH"
+wm title . "Git Authentication"
tk::PlaceWindow .
vwait rc
exit $rc
--
https://github.com/git/git/pull/195
^ permalink raw reply related
* Re: [PATCH] t*: avoid using pipes
From: Stefan Beller @ 2017-03-07 17:21 UTC (permalink / raw)
To: Prathamesh Chavan; +Cc: git@vger.kernel.org, Pranit Bauva
In-Reply-To: <CAME+mvUe7itzg7JLu9_131smzHHE0JsN-z7q8_dTY1qEdugYWw@mail.gmail.com>
On Tue, Mar 7, 2017 at 8:10 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> Hi,
> I'm Prathamesh Chavan. As a part of my micropraoject I have been working on
> "Avoid pipes for git related commands in test suites".
Thanks for working on that microproject!
> I tried sending the
> patch, but it got blocked since the mail contained more than 100 000
> characters.
Yeah, even the github UI seems to have trouble with that commit.
(A bit slow, not showing the full content, but rather I needed to click
on "load diff" for tests files 7000+)
This is a lot of change (in terms of lines) for a micro project. :)
I'd have two competing advices:
* 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.
The outcome will be the same, but in the first you get feedback
quicker, such that hopefully you only need to touch the rest of
files after the first file just once.
> Hence I'll like to attach the link to my branch 'micro-proj', where I did the
> required changes.
>
> 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)
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.
There is a difference in readability, though. For consistency I'd
suggest to drop the "<", as the numbers might support:
$ cd t
$ git grep head |wc -l
# This also counts other occurrences of the string,
# not just the invocation of the head tool
2871
$ git grep head |grep "<" |wc -l
# same here
58
Another aspect might be performance at scale as the "<" will
let the shell open the file and pipe the content via stdin to the
head tool, whereas when giving a filename the head tool needs
to open the file. Both times the file doesn't need to be read completely,
but "head -n 1" can close the file handle early in the game.
I dunno.
Thanks,
Stefan
>
> Thanks.
^ permalink raw reply
* [PATCH] repack: Add options to preserve and prune old pack files
From: James Melvin @ 2017-03-07 16:40 UTC (permalink / raw)
To: git; +Cc: nasserg, mfick, peff, sbeller, James Melvin
The new --preserve-oldpacks option moves old pack files into the
preserved subdirectory instead of deleting them after repacking.
The new --prune-preserved option prunes old pack files from the
preserved subdirectory after repacking, but before potentially
moving the latest old packfiles to this subdirectory.
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).
Signed-off-by: James Melvin <jmelvin@codeaurora.org>
---
Documentation/git-repack.txt | 9 +++++++++
builtin/repack.c | 38 ++++++++++++++++++++++++++++++++++++--
2 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index 26afe6ed5..0b19b761f 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -143,6 +143,15 @@ other objects in that pack they already have locally.
being removed. In addition, any unreachable loose objects will
be packed (and their loose counterparts removed).
+--preserve-oldpacks::
+ Move old pack files into the preserved subdirectory instead
+ of deleting them after repacking.
+
+--prune-preserved::
+ Prune old pack files from the preserved subdirectory after
+ repacking, but before potentially moving the latest old
+ packfiles to this subdirectory
+
Configuration
-------------
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;
static int write_bitmaps;
-static char *packdir, *packtmp;
+static char *packdir, *packtmp, *preservedir;
static const char *const git_repack_usage[] = {
N_("git repack [<options>]"),
@@ -108,6 +110,27 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list)
closedir(dir);
}
+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"));
+
+ fname_old = mkpathdup("%s/%s.old-%s", preservedir, file_name, ++file_ext);
+ rename(file_path, fname_old);
+
+ free(fname_old);
+}
+
+static void remove_preserved_dir(void) {
+ struct strbuf buf = STRBUF_INIT;
+
+ strbuf_addstr(&buf, preservedir);
+ remove_dir_recursively(&buf, 0);
+ strbuf_release(&buf);
+}
+
static void remove_redundant_pack(const char *dir_name, const char *base_name)
{
const char *exts[] = {".pack", ".idx", ".keep", ".bitmap"};
@@ -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);
}
strbuf_release(&buf);
}
@@ -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();
+
if (delete_redundant) {
int opts = 0;
string_list_sort(&names);
--
2.12.0.190.gab6997d48.dirty
^ permalink raw reply related
* [PATCH] t*: avoid using pipes
From: Prathamesh Chavan @ 2017-03-07 16:10 UTC (permalink / raw)
To: git; +Cc: Pranit Bauva
Hi,
I'm Prathamesh Chavan. As a part of my micropraoject I have been working on
"Avoid pipes for git related commands in test suites". I tried sending the
patch, but it got blocked since the mail contained more than 100 000
characters.
Hence I'll like to attach the link to my branch 'micro-proj', where I did the
required changes.
https://github.com/pratham-pc/git/tree/micro-proj
Thanks.
^ permalink raw reply
* Re: Reg : GSoC 2017 Microproject
From: Prathamesh Chavan @ 2017-03-07 15:51 UTC (permalink / raw)
To: Vedant Bassi; +Cc: git
In-Reply-To: <CACczA6WCdu0sdd31R2Z6xbr=meo5PTtcOVYCdVHdgZXAfK-3rg@mail.gmail.com>
On Tue, Mar 7, 2017 at 3:52 PM, 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 ?
>
> 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.
>
> Thanks.
You can search your microproject on public inbox of git to check if it
has already
been taken or not.
^ permalink raw reply
* Re: [PATCH v3 0/9] Fix the early config
From: Johannes Schindelin @ 2017-03-07 15:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Duy Nguyen
In-Reply-To: <xmqqpohy6o2a.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, 3 Mar 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > Notable notes:
> >
> > - In contrast to earlier versions, I no longer special-case init and
> > clone. Peff pointed out that this adds technical debt, and that we can
> > actually argue (for consistency's sake) that early config reads the
> > current repository config (if any) even for init and clone.
> >
> > - The read_early_config() function does not cache Git directory
> > discovery nor read values. If needed, this can be implemented later,
> > in a separate patch series.
> >
> > - The alias handling in git.c could possibly benefit from this work, but
> > again, this is a separate topic from the current patch series.
>
> As Peff said in his review, I too find the result of this series a
> more pleasant read than than original.
As do I.
> 2/9 and corresponding 4/9 triggers "ERROR: trailing statements
> should be on next line" from ../linux/scripts/checkpatch.pl because
> of a line inherited from the original; I'll queue them with an
> obvious style fix to work it around.
Thank you. I'll try to pick it up for v3 (which is needed, as I found
another issue that needs to be fixed).
Ciao,
Johannes
^ permalink raw reply
* [PATCH v4 07/10] read_early_config(): avoid .git/config hack when unneeded
From: Johannes Schindelin @ 2017-03-07 14:33 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488897111.git.johannes.schindelin@gmx.de>
So far, we only look whether the startup_info claims to have seen a
git_dir.
However, do_git_config_sequence() (and consequently the
git_config_with_options() call used by read_early_config() asks the
have_git_dir() function whether we have a .git/ directory, which in turn
also looks at git_dir and at the environment variable GIT_DIR. And when
this is the case, the repository config is handled already, so we do not
have to do that again explicitly.
Let's just use the same function, have_git_dir(), to determine whether we
have to handle .git/config explicitly.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
config.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index 9cfbeafd04c..068fa4dcfa6 100644
--- a/config.c
+++ b/config.c
@@ -1427,14 +1427,15 @@ void read_early_config(config_fn_t cb, void *data)
* core.repositoryformatversion), we'll read whatever is in .git/config
* blindly. Similarly, if we _are_ in a repository, but not at the
* root, we'll fail to find .git/config (because it's really
- * ../.git/config, etc). See t7006 for a complete set of failures.
+ * ../.git/config, etc), unless setup_git_directory() was already called.
+ * See t7006 for a complete set of failures.
*
* However, we have historically provided this hack because it does
* work some of the time (namely when you are at the top-level of a
* valid repository), and would rarely make things worse (i.e., you do
* not generally have a .git/config file sitting around).
*/
- if (!startup_info->have_repository) {
+ if (!have_git_dir()) {
struct git_config_source repo_config;
memset(&repo_config, 0, sizeof(repo_config));
--
2.12.0.windows.1.7.g94dafc3b124
^ 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