* [PATCH 11/11] refs: split and make get_*_ref_store() public API
From: Nguyễn Thái Ngọc Duy @ 2017-02-13 15:20 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Michael Haggerty, Johannes Schindelin, sbeller,
novalis, Nguyễn Thái Ngọc Duy
In-Reply-To: <20170213152011.12050-1-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
refs.c | 59 ++++++++++++++++++++++++++++++----------------------
refs.h | 13 ++++++++++++
refs/files-backend.c | 2 +-
refs/refs-internal.h | 12 -----------
4 files changed, 48 insertions(+), 38 deletions(-)
diff --git a/refs.c b/refs.c
index 9ac194945..48350da87 100644
--- a/refs.c
+++ b/refs.c
@@ -1160,7 +1160,7 @@ int head_ref(each_ref_fn fn, void *cb_data)
static int do_for_each_ref(const char *submodule, const char *prefix,
each_ref_fn fn, int trim, int flags, void *cb_data)
{
- struct ref_store *refs = get_ref_store(submodule);
+ struct ref_store *refs = get_submodule_ref_store(submodule);
struct ref_iterator *iter;
if (!refs)
@@ -1304,7 +1304,7 @@ const char *resolve_ref_recursively(struct ref_store *refs,
/* backend functions */
int refs_init_db(struct strbuf *err)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->init_db(refs, err);
}
@@ -1312,7 +1312,7 @@ int refs_init_db(struct strbuf *err)
const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
unsigned char *sha1, int *flags)
{
- return resolve_ref_recursively(get_ref_store(NULL), refname,
+ return resolve_ref_recursively(get_main_ref_store(), refname,
resolve_flags, sha1, flags);
}
@@ -1333,10 +1333,10 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
/* We need to strip off one or more trailing slashes */
char *stripped = xmemdupz(submodule, len);
- refs = get_ref_store(stripped);
+ refs = get_submodule_ref_store(stripped);
free(stripped);
} else {
- refs = get_ref_store(submodule);
+ refs = get_submodule_ref_store(submodule);
}
if (!refs)
@@ -1491,15 +1491,24 @@ static struct ref_store *init_submodule_ref_store(const char *path)
return refs;
}
-struct ref_store *get_ref_store(const char *submodule)
+struct ref_store *get_main_ref_store(void)
{
struct ref_store *refs;
- if (!submodule || !*submodule) {
- refs = lookup_ref_store(NULL);
+ refs = lookup_ref_store(NULL);
- if (!refs)
- refs = ref_store_init(NULL, NULL);
+ if (!refs)
+ refs = ref_store_init(NULL, NULL);
+
+ return refs;
+}
+
+struct ref_store *get_submodule_ref_store(const char *submodule)
+{
+ struct ref_store *refs;
+
+ if (!submodule || !*submodule) {
+ return get_main_ref_store();
} else {
refs = lookup_ref_store(submodule);
@@ -1519,14 +1528,14 @@ void base_ref_store_init(struct ref_store *refs,
/* backend functions */
int pack_refs(unsigned int flags)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->pack_refs(refs, flags);
}
int peel_ref(const char *refname, unsigned char *sha1)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->peel_ref(refs, refname, sha1);
}
@@ -1534,7 +1543,7 @@ int peel_ref(const char *refname, unsigned char *sha1)
int create_symref(const char *ref_target, const char *refs_heads_master,
const char *logmsg)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->create_symref(refs, ref_target, refs_heads_master,
logmsg);
@@ -1543,7 +1552,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master,
int ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->transaction_commit(refs, transaction, err);
}
@@ -1553,14 +1562,14 @@ int verify_refname_available(const char *refname,
const struct string_list *skip,
struct strbuf *err)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->verify_refname_available(refs, refname, extra, skip, err);
}
int for_each_reflog(each_ref_fn fn, void *cb_data)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
struct ref_iterator *iter;
iter = refs->be->reflog_iterator_begin(refs);
@@ -1571,7 +1580,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn,
void *cb_data)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->for_each_reflog_ent_reverse(refs, refname,
fn, cb_data);
@@ -1580,14 +1589,14 @@ int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn,
int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn,
void *cb_data)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->for_each_reflog_ent(refs, refname, fn, cb_data);
}
int reflog_exists(const char *refname)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->reflog_exists(refs, refname);
}
@@ -1595,14 +1604,14 @@ int reflog_exists(const char *refname)
int safe_create_reflog(const char *refname, int force_create,
struct strbuf *err)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->create_reflog(refs, refname, force_create, err);
}
int delete_reflog(const char *refname)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->delete_reflog(refs, refname);
}
@@ -1614,7 +1623,7 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
reflog_expiry_cleanup_fn cleanup_fn,
void *policy_cb_data)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->reflog_expire(refs, refname, sha1, flags,
prepare_fn, should_prune_fn,
@@ -1624,21 +1633,21 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
int initial_ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->initial_transaction_commit(refs, transaction, err);
}
int delete_refs(struct string_list *refnames, unsigned int flags)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->delete_refs(refs, refnames, flags);
}
int rename_ref(const char *oldref, const char *newref, const char *logmsg)
{
- struct ref_store *refs = get_ref_store(NULL);
+ struct ref_store *refs = get_main_ref_store();
return refs->be->rename_ref(refs, oldref, newref, logmsg);
}
diff --git a/refs.h b/refs.h
index 694784391..f5fba16c8 100644
--- a/refs.h
+++ b/refs.h
@@ -553,4 +553,17 @@ int reflog_expire(const char *refname, const unsigned char *sha1,
int ref_storage_backend_exists(const char *name);
+/*
+ * Return the ref_store instance for the specified submodule. For the
+ * main repository, use submodule==NULL; such a call cannot fail. For
+ * a submodule, the submodule must exist and be a nonbare repository,
+ * otherwise return NULL. If the requested reference store has not yet
+ * been initialized, initialize it first.
+ *
+ * For backwards compatibility, submodule=="" is treated the same as
+ * submodule==NULL.
+ */
+struct ref_store *get_submodule_ref_store(const char *submodule);
+struct ref_store *get_main_ref_store(void);
+
#endif /* REFS_H */
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2fb270b6f..eab156733 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3116,7 +3116,7 @@ int set_worktree_head_symref(const char *gitdir, const char *target)
* backends. This function needs to die.
*/
struct files_ref_store *refs =
- files_downcast(get_ref_store(NULL), "set_head_symref");
+ files_downcast(get_main_ref_store(), "set_head_symref");
static struct lock_file head_lock;
struct ref_lock *lock;
struct strbuf head_path = STRBUF_INIT;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index d7112770d..cb6882779 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -634,18 +634,6 @@ struct ref_store {
void base_ref_store_init(struct ref_store *refs,
const struct ref_storage_be *be);
-/*
- * Return the ref_store instance for the specified submodule. For the
- * main repository, use submodule==NULL; such a call cannot fail. For
- * a submodule, the submodule must exist and be a nonbare repository,
- * otherwise return NULL. If the requested reference store has not yet
- * been initialized, initialize it first.
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
- */
-struct ref_store *get_ref_store(const char *submodule);
-
const char *resolve_ref_recursively(struct ref_store *refs,
const char *refname,
int resolve_flags,
--
2.11.0.157.gd943d85
^ permalink raw reply related
* [PATCH 08/11] refs.c: factor submodule code out of get_ref_store()
From: Nguyễn Thái Ngọc Duy @ 2017-02-13 15:20 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Michael Haggerty, Johannes Schindelin, sbeller,
novalis, Nguyễn Thái Ngọc Duy
In-Reply-To: <20170213152011.12050-1-pclouds@gmail.com>
This code is going to be expanded a bit soon. Keep it out for
better readability later.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
refs.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/refs.c b/refs.c
index 2cacd934e..8ef7a52ba 100644
--- a/refs.c
+++ b/refs.c
@@ -1445,6 +1445,18 @@ static struct ref_store *lookup_ref_store(const char *submodule)
return entry ? entry->refs : NULL;
}
+static struct ref_store *init_submodule_ref_store(const char *submodule)
+{
+ struct strbuf submodule_sb = STRBUF_INIT;
+ struct ref_store *refs = NULL;
+
+ strbuf_addstr(&submodule_sb, submodule);
+ if (is_nonbare_repository_dir(&submodule_sb))
+ refs = ref_store_init(submodule);
+ strbuf_release(&submodule_sb);
+ return refs;
+}
+
struct ref_store *get_ref_store(const char *submodule)
{
struct ref_store *refs;
@@ -1457,14 +1469,8 @@ struct ref_store *get_ref_store(const char *submodule)
} else {
refs = lookup_ref_store(submodule);
- if (!refs) {
- struct strbuf submodule_sb = STRBUF_INIT;
-
- strbuf_addstr(&submodule_sb, submodule);
- if (is_nonbare_repository_dir(&submodule_sb))
- refs = ref_store_init(submodule);
- strbuf_release(&submodule_sb);
- }
+ if (!refs)
+ refs = init_submodule_ref_store(submodule);
}
return refs;
--
2.11.0.157.gd943d85
^ permalink raw reply related
* Re: Bug in 'git describe' if I have two tags on the same commit.
From: Kevin Daudt @ 2017-02-13 15:44 UTC (permalink / raw)
To: Istvan Pato; +Cc: git
In-Reply-To: <CAOcUJQwnCJOhUUU2RqJP2H5YxUr4qCEpyDj_XiiQSe4V6rcBmg@mail.gmail.com>
On Sun, Feb 12, 2017 at 01:15:22PM +0100, Istvan Pato wrote:
> I didn't get back the latest tag by 'git describe --tags --always' if
> I have two tags on the same commit.
>
> // repository ppa:git-core/ppa
>
> (master)⚡ % cat /etc/lsb-release
> DISTRIB_ID=Ubuntu
> DISTRIB_RELEASE=16.04
> DISTRIB_CODENAME=xenial
> DISTRIB_DESCRIPTION="Ubuntu 16.04.2 LTS"
>
> (master)⚡ % git --version
> git version 2.11.0
>
> (master) [1] % git show-ref --tag
> 76c634390... refs/tags/1.0.0
> b77c7cd17... refs/tags/1.1.0
> b77c7cd17... refs/tags/1.2.0
>
> (master) % git describe --tags --always
> 1.1.0-1-ge9e9ced
>
> ### Expected: 1.2.0
>
> References:
>
> https://www.kernel.org/pub/software/scm/git/docs/RelNotes-1.7.1.1.txt
>
> * "git describe" did not tie-break tags that point at the same commit
> correctly; newer ones are preferred by paying attention to the
> tagger date now.
>
> http://stackoverflow.com/questions/8089002/git-describe-with-two-tags-on-the-same-commit
>
> Thanks,
> Istvan Pato
Are these lightweight tags? Only annotated tags have a date associated
to them, which is where the rel-notes refers to.
^ permalink raw reply
* Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C
From: Johannes Schindelin @ 2017-02-13 16:23 UTC (permalink / raw)
To: René Scharfe; +Cc: Pranit Bauva, git, Junio C Hamano
In-Reply-To: <128b4de6-7b8e-27b9-414d-c6c6529cb491@web.de>
[-- Attachment #1: Type: text/plain, Size: 1670 bytes --]
Hi René,
On Fri, 10 Feb 2017, René Scharfe wrote:
> Am 10.02.2017 um 15:20 schrieb Johannes Schindelin:
> > It is curious that only MacOSX builds trigger an error about this, both
> > GCC and Clang, but not Linux GCC nor Clang (see
> > https://travis-ci.org/git/git/jobs/200182819#L1152 for details):
> >
> > builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used
> > uninitialized whenever 'if' condition is true
> > [-Werror,-Wsometimes-uninitialized]
> > if (missing_good && !missing_bad && current_term &&
> > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > builtin/bisect--helper.c:350:7: note: uninitialized use occurs here
> > if (!good_syn)
> > ^~~~~~~~
>
> The only way that good_syn could be used in the if block is by going to the
> label finish, which does the following before returning:
>
> if (!bad_ref)
> free(bad_ref);
> if (!good_glob)
> free(good_glob);
> if (!bad_syn)
> free(bad_syn);
> if (!good_syn)
> free(good_syn);
>
> On Linux that code is elided completely -- freeing NULL is a no-op. I guess
> free(3) has different attributes on OS X and compilers don't dare to optimize
> it away there.
>
> So instead of calling free(3) only in the case when we did not allocate memory
> (which makes no sense and leaks) we should either call it in the opposite
> case, or (preferred) unconditionally, as it can handle the NULL case itself.
> Once that's fixed initialization will be required even on Linux.
Exactly, free(NULL) is a no-op. The problem before this fixup was that
good_syn was not initialized to NULL.
Ciao,
Dscho
^ permalink raw reply
* git remote rename problem with trailing \\ for remote.url config entries (on Windows)
From: Marc Strapetz @ 2017-02-13 16:49 UTC (permalink / raw)
To: git
One of our users has just reported that:
$ git remote rename origin origin2
will turn following remote entry:
[remote "origin"]
url = c:\\repo\\
fetch = +refs/heads/*:refs/remotes/origin/*
into following entry for which the url is skipped:
[remote "origin2"]
[remote "origin2"]
fetch = +refs/heads/*:refs/remotes/origin2/*
I understand that this is caused by the trailing \\ and it's easy to
fix, but 'git push' and 'git pull' work properly with such URLs and a
$ git clone c:\repo\
will even result in the problematic remote-entry. So I guess some kind
of validation could be helpful.
Tested with git version 2.11.0.windows.1
-Marc
^ permalink raw reply
* Re: [PATCH] mingw: use OpenSSL's SHA-1 routines
From: Johannes Schindelin @ 2017-02-13 17:16 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, git, Jeff Hostetler
In-Reply-To: <31bb0b9f-d498-24b3-57d5-9f34cb8e3914@kdbg.org>
Hi Hannes,
On Sat, 11 Feb 2017, Johannes Sixt wrote:
> Am 10.02.2017 um 00:41 schrieb Junio C Hamano:
> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >
> > > From: Jeff Hostetler <jeffhost@microsoft.com>
> > >
> > > Use OpenSSL's SHA-1 routines rather than builtin block-sha1
> > > routines. This improves performance on SHA1 operations on Intel
> > > processors.
> > > ...
> > >
> > > Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > > ---
> >
> > Nice. Will queue as jh/mingw-openssl-sha1 topic; it is a bit too
> > late for today's integration cycle to be merged to 'next', but let's
> > have this by the end of the week in 'master'.
>
> Please don't rush this through. I didn't have a chance to cross-check the
> patch; it will have to wait for Monday. I would like to address Peff's
> concerns about additional runtime dependencies.
I never meant this to be fast-tracked into git.git. We have all the time
in our lives to get this in, as Git for Windows already carries this patch
for a while, and shall continue to do so.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH] mingw: use OpenSSL's SHA-1 routines
From: Johannes Sixt @ 2017-02-13 17:46 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, git, Jeff Hostetler, Junio C Hamano
In-Reply-To: <20170210160458.pcp7mupdz24m6cms@sigill.intra.peff.net>
Am 10.02.2017 um 17:04 schrieb Jeff King:
> On Fri, Feb 10, 2017 at 04:49:02PM +0100, Johannes Schindelin wrote:
>
>>> I think this is only half the story. A heavy-sha1 workload is faster,
>>> which is good. But one of the original reasons to prefer blk-sha1 (at
>>> least on Linux) is that resolving libcrypto.so symbols takes a
>>> non-trivial amount of time. I just timed it again, and it seems to be
>>> consistently 1ms slower to run "git rev-parse --git-dir" on my machine
>>> (from the top-level of a repo).
>>>
>>> 1ms is mostly irrelevant, but it adds up on scripted workloads that
>>> start a lot of git processes.
>>
>> You know my answer to that. If scripting slows things down, we should
>> avoid it in production code. As it is, scripting slows us down. Therefore
>> I work slowly but steadily to get rid of scripting where it hurts most.
>
> Well, yes. My question is more "what does it look like on normal Git
> workloads?". Are you trading off an optimization for your giant 450MB
> index workload (which _also_ could be fixed by trying do the slow
> operation less, rather than micro-optimizing it) in a way that hurts
> people working with more normal sized repos?
>
> For instance, "make BLK_SHA1=Yes test" is measurably faster for me than
> "make BLK_SHA1= test".
The patch does add a new runtime dependency on libcrypto.dll in my
environment. I would be surprised if it does not also with your modern
build tools.
I haven't had time to compare test suite runtimes.
> I'm open to the argument that it doesn't matter in practice for normal
> git users. But it's not _just_ scripting. It depends on the user's
> pattern of invoking git commands (and how expensive the symbol
> resolution is on their system).
It can be argued that in normal interactive use, it is hard to notice
that another DLL is loaded. Don't forget, though, that on Windows it is
not only the pure time to resolve the entry points, but also that
typically virus scanners inspect every executable file that is loaded,
which adds another share of time.
I'll use the patch for daily work for a while to see whether it hurts.
-- Hannes
^ permalink raw reply
* Small regression on Windows
From: Johannes Sixt @ 2017-02-13 18:00 UTC (permalink / raw)
To: Johannes Schindelin, Jeff Hostetler; +Cc: Junio C Hamano, Git Mailing List
Please type this, preferably outside of any repo to avoid that it is
changed; note the command typo:
git -c help.autocorrect=40 ceckout
Git waits for 4 seconds, but does not write anything until just before
it exits. It bisects to
a9b8a09c3c30886c79133da9f48ef9f98c21c3b2 is the first bad commit
commit a9b8a09c3c30886c79133da9f48ef9f98c21c3b2
Author: Jeff Hostetler <jeffhost@microsoft.com>
Date: Thu Dec 22 18:09:23 2016 +0100
mingw: replace isatty() hack
Git for Windows has carried a patch that depended on internals
of MSVC runtime, but it does not work correctly with recent MSVC
runtime. A replacement was written originally for compiling
with VC++. The patch in this message is a backport of that
replacement, and it also fixes the previous attempt to make
isatty() tell that /dev/null is *not* an interactive terminal.
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Tested-by: Beat Bolli <dev+git@drbeat.li>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
:040000 040000 bc3c75bdfc766fe478e160a9452c31bfef50b505
f7183c3a0726fef7161d5f9ff8c997260025f1bb M compat
Any ideas? I haven't had time to dig any further.
-- Hannes
^ permalink raw reply
* Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C
From: René Scharfe @ 2017-02-13 18:27 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Pranit Bauva, git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1702131722350.3496@virtualbox>
Am 13.02.2017 um 17:23 schrieb Johannes Schindelin:
> Hi René,
>
> On Fri, 10 Feb 2017, René Scharfe wrote:
>
>> Am 10.02.2017 um 15:20 schrieb Johannes Schindelin:
>>> It is curious that only MacOSX builds trigger an error about this, both
>>> GCC and Clang, but not Linux GCC nor Clang (see
>>> https://travis-ci.org/git/git/jobs/200182819#L1152 for details):
>>>
>>> builtin/bisect--helper.c:299:6: error: variable 'good_syn' is used
>>> uninitialized whenever 'if' condition is true
>>> [-Werror,-Wsometimes-uninitialized]
>>> if (missing_good && !missing_bad && current_term &&
>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> builtin/bisect--helper.c:350:7: note: uninitialized use occurs here
>>> if (!good_syn)
>>> ^~~~~~~~
>>
>> The only way that good_syn could be used in the if block is by going to the
>> label finish, which does the following before returning:
>>
>> if (!bad_ref)
>> free(bad_ref);
>> if (!good_glob)
>> free(good_glob);
>> if (!bad_syn)
>> free(bad_syn);
>> if (!good_syn)
>> free(good_syn);
>>
>> On Linux that code is elided completely -- freeing NULL is a no-op. I guess
>> free(3) has different attributes on OS X and compilers don't dare to optimize
>> it away there.
>>
>> So instead of calling free(3) only in the case when we did not allocate memory
>> (which makes no sense and leaks) we should either call it in the opposite
>> case, or (preferred) unconditionally, as it can handle the NULL case itself.
>> Once that's fixed initialization will be required even on Linux.
>
> Exactly, free(NULL) is a no-op. The problem before this fixup was that
> good_syn was not initialized to NULL.
Strictly speaking: no. The value doesn't matter -- the free(3) calls
above will be done with NULL regardless, due to the conditionals.
Setting bad_syn and good_syn to an invalid pointer would have calmed
the compiler just as well, and would have had no ill side effect (i.e.
no invalid free(3) call).
Initializing to NULL is still the correct thing to do, of course --
together with removing the conditionals (or at least the negations).
But back to the topic of why the compilers on OS X didn't optimize out
the free(3) calls with their conditionals. AFAICS no attributes are
set for the function in stdlib.h of in glibc[1] or Darwin[2]. And I
can't see any relevant option in config.mak.uname (e.g. -no-builtin).
It's not terribly important, but does anyone know what prevents the
elision of "if (!p) free(p);" on OS X?
René
[1] https://sourceware.org/git/?p=glibc.git;a=blob;f=stdlib/stdlib.h;h=292c6a2f053a2a578cd09d75307c26ed191e1c00;hb=b987917e6aa7ffe2fd74f0b6a989438e6edd0727
[2] https://opensource.apple.com/source/Libc/Libc-1158.30.7/include/stdlib.h.auto.html
^ permalink raw reply
* Re: [PATCH] clean: use warning_errno() when appropriate
From: Junio C Hamano @ 2017-02-13 18:34 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <20170213092702.10462-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> All these warning() calls are preceded by a system call. Report the
> actual error to help the user understand why we fail to remove
> something.
I think this patch is probably correct in the current code, but I
say this only after following what quote_path_relative() and
relative_path() that is called from it. These warnings are preceded
by a call to a system library function, but it is not apparent that
they are immediately preceded without anything else that could have
failed in between.
Side note. There are many calls into strbuf API in these two
functions. Any calls to xmalloc() and friends made by strbuf
functions may see ENOMEM from underlying malloc() and recover by
releasing cached resources, by which time the original errno is
unrecoverable. So the above "probably correct" is not strictly
true.
If we care deeply enough that we want to reliably show the errno we
got from the preceding call to a system library function even after
whatever comes in between, I think you'd need the usual saved_errno
trick. Is that worth it?---I do not offhand have an opinion.
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> builtin/clean.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/clean.c b/builtin/clean.c
> index d6bc3aaaea..dc1168747e 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -175,7 +175,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
> res = dry_run ? 0 : rmdir(path->buf);
> if (res) {
> quote_path_relative(path->buf, prefix, "ed);
> - warning(_(msg_warn_remove_failed), quoted.buf);
> + warning_errno(_(msg_warn_remove_failed), quoted.buf);
> *dir_gone = 0;
> }
> return res;
> @@ -209,7 +209,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
> string_list_append(&dels, quoted.buf);
> } else {
> quote_path_relative(path->buf, prefix, "ed);
> - warning(_(msg_warn_remove_failed), quoted.buf);
> + warning_errno(_(msg_warn_remove_failed), quoted.buf);
> *dir_gone = 0;
> ret = 1;
> }
> @@ -231,7 +231,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
> *dir_gone = 1;
> else {
> quote_path_relative(path->buf, prefix, "ed);
> - warning(_(msg_warn_remove_failed), quoted.buf);
> + warning_errno(_(msg_warn_remove_failed), quoted.buf);
> *dir_gone = 0;
> ret = 1;
> }
> @@ -982,7 +982,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
> res = dry_run ? 0 : unlink(abs_path.buf);
> if (res) {
> qname = quote_path_relative(item->string, NULL, &buf);
> - warning(_(msg_warn_remove_failed), qname);
> + warning_errno(_(msg_warn_remove_failed), qname);
> errors++;
> } else if (!quiet) {
> qname = quote_path_relative(item->string, NULL, &buf);
^ permalink raw reply
* Re: git remote rename problem with trailing \\ for remote.url config entries (on Windows)
From: Junio C Hamano @ 2017-02-13 18:47 UTC (permalink / raw)
To: Marc Strapetz; +Cc: git, Johannes Schindelin
In-Reply-To: <da730b21-58ae-bfa8-705f-7669c0f56764@syntevo.com>
Marc Strapetz <marc.strapetz@syntevo.com> writes:
> One of our users has just reported that:
>
> $ git remote rename origin origin2
>
> will turn following remote entry:
>
> [remote "origin"]
> url = c:\\repo\\
> fetch = +refs/heads/*:refs/remotes/origin/*
>
> into following entry for which the url is skipped:
>
> [remote "origin2"]
> [remote "origin2"]
> fetch = +refs/heads/*:refs/remotes/origin2/*
>
> I understand that this is caused by the trailing \\ and it's easy to
> fix, but 'git push' and 'git pull' work properly with such URLs and a
>
> $ git clone c:\repo\
>
> will even result in the problematic remote-entry. So I guess some kind
> of validation could be helpful.
If you change the original definition of the "origin" to
[remote "origin"]
url = "c:\\repo\\"
fetch = +refs/heads/*:refs/remotes/origin/*
or
[remote "origin"]
url = c:\\repo\\ # ends with bs
fetch = +refs/heads/*:refs/remotes/origin/*
it seems to give me a better result. I didn't dig to determine
where things go wrong in "remote rename", and it is possible that
the problem is isolated to that command, or the same problem exists
with any value that ends with a backslash. If the latter, "git clone"
and anything that writes to configuration such a value needs to be
taught about this glitch and learn a workaround, I would think.
Dscho Cc'ed, not for Windows expertise, but as somebody who has done
a lot in <config.c>.
^ permalink raw reply
* Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C
From: Junio C Hamano @ 2017-02-13 19:14 UTC (permalink / raw)
To: René Scharfe; +Cc: Johannes Schindelin, Pranit Bauva, git
In-Reply-To: <74dfcffe-274c-7045-420a-95612394d66b@web.de>
René Scharfe <l.s.r@web.de> writes:
> Initializing to NULL is still the correct thing to do, of course --
> together with removing the conditionals (or at least the negations).
So, let's give Pranit a concrete "here is what we want to see
squashed in", while you guys discuss peculiarity with various
platforms and their system headers, which admittedly is a more
interesting tangent ;-)
There are early returns with "goto finish" even before _syn
variables are first assigned to, so they would need to be
initialized to NULL. The other two get their initial values
right at the beginning, so they are OK.
builtin/bisect--helper.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 8cd6527bd1..6949e8e5ca 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -280,7 +280,7 @@ static int bisect_next_check(const struct bisect_terms *terms,
int missing_good = 1, missing_bad = 1, retval = 0;
char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
char *good_glob = xstrfmt("%s-*", terms->term_good);
- char *bad_syn, *good_syn;
+ char *bad_syn = NULL, *good_syn = NULL;
if (ref_exists(bad_ref))
missing_bad = 0;
@@ -341,14 +341,10 @@ static int bisect_next_check(const struct bisect_terms *terms,
}
goto finish;
finish:
- if (!bad_ref)
- free(bad_ref);
- if (!good_glob)
- free(good_glob);
- if (!bad_syn)
- free(bad_syn);
- if (!good_syn)
- free(good_syn);
+ free(bad_ref);
+ free(good_glob);
+ free(bad_syn);
+ free(good_syn);
return retval;
}
^ permalink raw reply related
* Re: [PATCH] clean: use warning_errno() when appropriate
From: Jeff King @ 2017-02-13 19:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <xmqqwpcudjoh.fsf@gitster.mtv.corp.google.com>
On Mon, Feb 13, 2017 at 10:34:06AM -0800, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
> > All these warning() calls are preceded by a system call. Report the
> > actual error to help the user understand why we fail to remove
> > something.
>
> I think this patch is probably correct in the current code, but I
> say this only after following what quote_path_relative() and
> relative_path() that is called from it. These warnings are preceded
> by a call to a system library function, but it is not apparent that
> they are immediately preceded without anything else that could have
> failed in between.
>
> Side note. There are many calls into strbuf API in these two
> functions. Any calls to xmalloc() and friends made by strbuf
> functions may see ENOMEM from underlying malloc() and recover by
> releasing cached resources, by which time the original errno is
> unrecoverable. So the above "probably correct" is not strictly
> true.
>
> If we care deeply enough that we want to reliably show the errno we
> got from the preceding call to a system library function even after
> whatever comes in between, I think you'd need the usual saved_errno
> trick. Is that worth it?---I do not offhand have an opinion.
I wonder if xmalloc() should be the one doing the saved_errno trick.
After all, it has only two outcomes: we successfully allocated the
memory, or we called die().
And that would transitively make most of the strbuf calls errno-safe
(except for obvious syscall-related ones like strbuf_read_file). And in
turn that makes quote_path_relative() pretty safe (at least when writing
to a strbuf).
-Peff
^ permalink raw reply
* [PATCH] completion: restore removed line continuating backslash
From: SZEDER Gábor @ 2017-02-13 19:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, SZEDER Gábor
In-Reply-To: <20170203024829.8071-18-szeder.dev@gmail.com>
Recent commit 1cd23e9e0 (completion: don't use __gitdir() for git
commands, 2017-02-03) rewrapped a couple of long lines, and while
doing so it inadvertently removed a '\' from the end of a line, thus
breaking completion for 'git config remote.name.push <TAB>'.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
I wanted this to be a fixup!, but then noticed that this series is
already in next, hence the proper commit message.
I see the last What's cooking marks this series as "will merge to
master". That doesn't mean that it will be in v2.12, does it?
contrib/completion/git-completion.bash | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 993085af6..f2b294aac 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2086,7 +2086,7 @@ _git_config ()
remote.*.push)
local remote="${prev#remote.}"
remote="${remote%.push}"
- __gitcomp_nl "$(__git for-each-ref
+ __gitcomp_nl "$(__git for-each-ref \
--format='%(refname):%(refname)' refs/heads)"
return
;;
--
2.11.1.499.ge87add2e7
^ permalink raw reply related
* Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]
From: Junio C Hamano @ 2017-02-13 19:21 UTC (permalink / raw)
To: Arif Khokar
Cc: Johannes Schindelin, Arif Khokar, Jakub Narębski, Jeff King,
Stefan Beller, meta@public-inbox.org, git@vger.kernel.org,
Eric Wong
In-Reply-To: <d16546a4-25be-7b85-3191-e9393fda1164@hotmail.com>
Arif Khokar <arif.i.khokar@gmail.com> writes:
> ... I
> still think it would be better to be able to list the message-id
> values in the header or body of the cover letter message of a patch
> series (preferably the former) in order to facilitate downloading the
> patches via NNTP from gmane or public-inbox.org.
You are looking at builtin/log.c::make_cover_letter()? Patches
welcome, but I think you'd need a bit of preparatory refactoring
to allow gen_message_id() be called for all messages _before_ this
function is called, as currently we generate them as we emit each
patch.
> Alternatively, or perhaps in addition to the list of message-ids, a
> list of URLs to public-inbox.org or gmane messages could also be
> provided for those who prefer to download patches via HTTP.
Many people around here do not want to repeat the mistake of relying
too much on one provider. Listing Message-IDs may be a good idea,
listing URLs that are tied to one provider is less so.
^ permalink raw reply
* Re: Incorrect pipe for git log graph when using --name-status option
From: Junio C Hamano @ 2017-02-13 19:25 UTC (permalink / raw)
To: hIpPy; +Cc: git
In-Reply-To: <CAM_JFCwYAKCWrHqCtcwid27V1HvhuSmp4QpbNpgyMzrzWUNYog@mail.gmail.com>
hIpPy <hippy2981@gmail.com> writes:
> The `git log` command with `graph` and pretty format works correctly
> as expected.
>
> $ git log --graph --pretty=format:'%h' -2
> * 714a14e
> * 87dce5f
>
>
> However, with `--name-status` option added, there is a pipe
> incorrectly placed after the commit hash (example below).
>
> $ git log --graph --pretty=format:'%h' -2 --name-status
> * 714a14e|
> | M README.md
Is it a --name-status, or is it your own custom format, that is
causing the above issue?
- What happens if you stop using --pretty=format:%h and replace it
with something like --oneline?
- What happens if you keep using --pretty=format:%h but replace
--name-status with something else, e.g. --raw or --stat?
^ permalink raw reply
* Re: [PATCH] squash! completion: fill COMPREPLY directly when completing refs
From: SZEDER Gábor @ 2017-02-13 19:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git mailing list
In-Reply-To: <xmqq7f4xk9es.fsf@gitster.mtv.corp.google.com>
On Fri, Feb 10, 2017 at 10:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Should I expect a reroll to come, or is this the only fix-up to the
> series that begins at <20170203025405.8242-1-szeder.dev@gmail.com>?
>
> No hurries.
Yes, definitely.
I found a minor bug in the middle of the series, and haven't quite
made up my mind about it and its fix yet. Perhaps in a day or three.
Regarding the 'strip' -> 'lstrip' 'for-each-ref' format modifier
rename that broke pu: I should keep using 'strip', right?
Gábor
^ permalink raw reply
* Re: [RFC PATCH] show decorations at the end of the line
From: Linus Torvalds @ 2017-02-13 19:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <xmqq1sv2fq6m.fsf@gitster.mtv.corp.google.com>
On Mon, Feb 13, 2017 at 12:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> An obvious downside is that people (against all recommendations) are
> likely to have written a loose script expecting the --oneline format
> is cast in stone.
Actually, I don't believe that is the case wrt decorations.
Why?
If you script the --oneline format and parse the output, you won't
have any decorations at all unless you are crazy (you can set
"log.decorations=true", but that will truly screw up any scripting).
And if you actually want decorations, and you're parsing them, you are
*not* going to script it with "--oneline --decorations", because the
end result is basically impossible to parse already (because it's
ambiguous - think about parentheses in the commit message).
So if you actually want decorations for parsing, you'd do something like
git log --pretty="%h '%D' %s"
which is at least parseable (because now the decoration separator is
unconditional.
Yeah, I guess you could use "--decorations --color=always" and then
use the color codes to parse the decorations, but that's so
complicated as to be unrealistic.
And I considered adding a format string explanation, something along
the lines of
- oneline used to mean "--pretty=%h%d %s", now it means "%h %s%d" instead
but that's actually not true. The "oneline" format was much more
complex than that, in that it has special rules for "-g", and it has
all those colorization ones too.
Linus
^ permalink raw reply
* Re: [PATCH] fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C
From: Pranit Bauva @ 2017-02-13 19:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, Johannes Schindelin, Git List
In-Reply-To: <xmqqinodewdr.fsf@gitster.mtv.corp.google.com>
Hey Junio,
On Tue, Feb 14, 2017 at 12:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> René Scharfe <l.s.r@web.de> writes:
>
>> Initializing to NULL is still the correct thing to do, of course --
>> together with removing the conditionals (or at least the negations).
>
> So, let's give Pranit a concrete "here is what we want to see
> squashed in", while you guys discuss peculiarity with various
> platforms and their system headers, which admittedly is a more
> interesting tangent ;-)
>
> There are early returns with "goto finish" even before _syn
> variables are first assigned to, so they would need to be
> initialized to NULL. The other two get their initial values
> right at the beginning, so they are OK.
>
> builtin/bisect--helper.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 8cd6527bd1..6949e8e5ca 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -280,7 +280,7 @@ static int bisect_next_check(const struct bisect_terms *terms,
> int missing_good = 1, missing_bad = 1, retval = 0;
> char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
> char *good_glob = xstrfmt("%s-*", terms->term_good);
> - char *bad_syn, *good_syn;
> + char *bad_syn = NULL, *good_syn = NULL;
>
> if (ref_exists(bad_ref))
> missing_bad = 0;
> @@ -341,14 +341,10 @@ static int bisect_next_check(const struct bisect_terms *terms,
> }
> goto finish;
> finish:
> - if (!bad_ref)
> - free(bad_ref);
> - if (!good_glob)
> - free(good_glob);
> - if (!bad_syn)
> - free(bad_syn);
> - if (!good_syn)
> - free(good_syn);
> + free(bad_ref);
> + free(good_glob);
> + free(bad_syn);
> + free(good_syn);
> return retval;
> }
This helps a lot ;) Thanks!
Regards,
Pranit Bauva
^ permalink raw reply
* Re: [PATCH] mingw: use OpenSSL's SHA-1 routines
From: Junio C Hamano @ 2017-02-13 19:42 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, Jeff King, git, Jeff Hostetler
In-Reply-To: <9913e513-553e-eba6-e81a-9c21030dd767@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> The patch does add a new runtime dependency on libcrypto.dll in my
> environment. I would be surprised if it does not also with your modern
> build tools.
>
> I haven't had time to compare test suite runtimes.
>
>> I'm open to the argument that it doesn't matter in practice for normal
>> git users. But it's not _just_ scripting. It depends on the user's
>> pattern of invoking git commands (and how expensive the symbol
>> resolution is on their system).
>
> It can be argued that in normal interactive use, it is hard to notice
> that another DLL is loaded. Don't forget, though, that on Windows it
> is not only the pure time to resolve the entry points, but also that
> typically virus scanners inspect every executable file that is loaded,
> which adds another share of time.
>
> I'll use the patch for daily work for a while to see whether it hurts.
Thanks.
I need to ask an unrelated question at a bit higher level, though.
I have been operating under the assumption that everybody on Windows
who builds Git works off of Dscho's Git for Windows tree, and
patches that are specific to Windows from Dscho's are sent to me via
the list only after they have been in Git for Windows and proven to
help Windows users in the wild.
The consequence of these two assumptions is that I would feel safe
to treat Windows specific changes that do not touch generic part of
the codebase from Dscho just like updates from any other subsystem
maintainers (any git-svn thing from Eric, any gitk thing from Paul,
any p4 thing Luke and Lars are both happy with, etc.).
You seem to be saying that the first of the two assumptions does not
hold. Should I change my expectations while queuing Windows specific
patches from Dscho?
^ permalink raw reply
* Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"
From: Junio C Hamano @ 2017-02-13 19:51 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Siddharth Kannan, git, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <vpqbmu768on.fsf@anie.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
>
>> + if (!strcmp(name, "-")) {
>> + name = "@{-1}";
>> + len = 5;
>> + }
>
> One drawback of this approach is that further error messages will be
> given from the "@{-1}" string that the user never typed.
Right.
> There are at least:
>
> $ git grep -n -A1 'strcmp.*"-"' | grep -B 1 '@\{1\}'
> builtin/checkout.c:975: if (!strcmp(arg, "-"))
> builtin/checkout.c-976- arg = "@{-1}";
I didn't check the surrounding context to be sure, but I think this
"- to @{-1}" conversion cannot be delegated down to revision parsing
that eventually wants to return a 40-hex as the result.
We do want a branch _name_ sometimes when we say "@{-1}"; "checkout
master" (i.e. checkout by name) and "checkout master^0" (i.e. the
same commit object, but not by name) do different things.
> builtin/merge.c:1231: } else if (argc == 1 && !strcmp(argv[0], "-")) {
> builtin/merge.c-1232- argv[0] = "@{-1}";
> --
> builtin/revert.c:158: if (!strcmp(argv[1], "-"))
> builtin/revert.c-159- argv[1] = "@{-1}";
These should be safe to delegate down.
> builtin/worktree.c:344: if (!strcmp(branch, "-"))
> builtin/worktree.c-345- branch = "@{-1}";
I do not know about this one, but it smells like a branch name that
wants to be used before it gets turned into 40-hex.
^ permalink raw reply
* Re: [PATCH 2/2 v3] sha1_name: teach get_sha1_1 "-" shorthand for "@{-1}"
From: Junio C Hamano @ 2017-02-13 20:03 UTC (permalink / raw)
To: Siddharth Kannan, Matthieu Moy; +Cc: git, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <xmqq1sv1euob.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
>>
>>> + if (!strcmp(name, "-")) {
>>> + name = "@{-1}";
>>> + len = 5;
>>> + }
>>
>> One drawback of this approach is that further error messages will be
>> given from the "@{-1}" string that the user never typed.
>
> Right.
>
>> There are at least:
>>
>> $ git grep -n -A1 'strcmp.*"-"' | grep -B 1 '@\{1\}'
>> builtin/checkout.c:975: if (!strcmp(arg, "-"))
>> builtin/checkout.c-976- arg = "@{-1}";
>
> I didn't check the surrounding context to be sure, but I think this
> "- to @{-1}" conversion cannot be delegated down to revision parsing
> that eventually wants to return a 40-hex as the result.
>
> We do want a branch _name_ sometimes when we say "@{-1}"; "checkout
> master" (i.e. checkout by name) and "checkout master^0" (i.e. the
> same commit object, but not by name) do different things.
FYI, the "@{-<number>} to branch name" translation happens in
interpret_branch_name(). I do not offhand recall if any callers
protect their calls to the function with conditionals that assume
the thing must begin with "@{" or cannot begin with "-" (the latter
of which is similar to the topic of patch 1/2 of this series), but I
suspect that teaching the function that "-" means the same as
"@{-1}" would bring us closer to where we want to go.
^ permalink raw reply
* Re: [PATCH v3 0/5] stash: support pathspec argument
From: Jeff King @ 2017-02-13 20:09 UTC (permalink / raw)
To: Matthieu Moy
Cc: Thomas Gummerer, git, Stephan Beyer, Junio C Hamano,
Marc Strapetz, Johannes Schindelin, Øyvind A . Holm,
Jakub Narębski
In-Reply-To: <vpq7f4uxjmo.fsf@anie.imag.fr>
On Mon, Feb 13, 2017 at 03:14:55PM +0100, Matthieu Moy wrote:
> I don't remember doing this intentionally. The goal was really to
> prevent typos like "git stash -p drop" (which would have been
> interpreted as "git stash save -p drop" previously). So, let's consider
> this "only messages starting with - are accepted" as a bug: moving to
> the new "push" command it a nice opportunity to fix it without worrying
> about compatibility. A bit more about this in this old thread:
Yeah, I consider it a bug that we treat "-foo" as a message there.
Fortunately I think you really have to try hard to abuse it:
# doesn't work, because "save" complains about "-foo" as an option
git stash -foo
# does save stash with message "-foo"
git stash -- -foo
# doesn't work, because _any_ non-option argument is rejected
git stash -- use --foo option
> I think we can safely allow both
>
> git stash -p -- <pathspec...>
> git stash push -p <pathspec...>
>
> But allowing the same without -- is a bit more dangerous for the reason
> above:
>
> git stash -p drop
>
> would be interpreted as
>
> git stash push -p drop
>
> (i.e. limit the stash to file named "drop"), and this would almost
> certainly not be what the user wanted.
Is it really that dangerous, though? The likely outcome is Git saying
"nope, you don't have any changes to the file named drop". Of course the
user may have meant something different, but I feel like "-p" is a good
indicator that they are interested in making an actual stash.
Think about this continuum of commands:
1. git stash droop
2. git stash -p drop
3. git stash -p -- drop
4. git stash push -p drop
I think we can all agree that (4) is reasonable and safe. And I suspect
we'd all agree that (1) is suspect (you probably meant "drop", not "push
droop").
But between (2) and (3), I don't see much difference. The interesting
difference between (1) and (2) is the addition of "-p", which tells us
that the user expects to save a stash.
Another way of thinking of it is that "-p" as a command name is an alias
for "push -p". If you wanted to err on the conservative side, you could
literally implement it that way (so "git stash -k -p foo" would not
work; you'd have to use "git stash -p -k foo").
> > The other question is how we would deal with the -m flag for
> > specifying a stash message. I think we could special case it, but
> > that would allow users to do things such as git stash -m apply, which
> > would make the interface a bit more error prone. So I'm leaning
> > towards disallowing that for now.
>
> We already have "git commit -m <msg> <pathspec...>", so I think stash
> should just do the same thing as commit. Or, did I miss something?
The complexity is that right now, the first-level decision of "which
stash sub-command am I running?" doesn't know about any options. So "git
stash -m foo" would be rejected in the name of typo prevention, unless
that outer decision learns about "-m" as an option.
-Peff
^ permalink raw reply
* Re: [PATCH 01/11] refs-internal.c: make files_log_ref_write() static
From: Ramsay Jones @ 2017-02-13 20:14 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Michael Haggerty, Johannes Schindelin, sbeller,
novalis
In-Reply-To: <20170213152011.12050-2-pclouds@gmail.com>
On 13/02/17 15:20, Nguyễn Thái Ngọc Duy wrote:
> Created in 5f3c3a4e6f (files_log_ref_write: new function - 2015-11-10)
> but probably never used outside refs-internal.c
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> refs/files-backend.c | 3 +++
> refs/refs-internal.h | 4 ----
> 2 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index cdb6b8ff5..75565c3aa 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -165,6 +165,9 @@ static struct ref_entry *create_dir_entry(struct files_ref_store *ref_store,
> const char *dirname, size_t len,
> int incomplete);
> static void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry);
> +static int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
> + const unsigned char *new_sha1, const char *msg,
> + int flags, struct strbuf *err);
Why? I don't like adding forward declarations unless it
is absolutely necessary (ie mutually recursive functions),
and even in the current 'pu' branch (@c04899d50), the
definition of this function appears before all uses in
this file. (ie, just add static to the definition).
What am I missing?
ATB,
Ramsay Jones
>
> static struct ref_dir *get_ref_dir(struct ref_entry *entry)
> {
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 33adbf93b..59e65958a 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -222,10 +222,6 @@ struct ref_transaction {
> enum ref_transaction_state state;
> };
>
> -int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
> - const unsigned char *new_sha1, const char *msg,
> - int flags, struct strbuf *err);
> -
> /*
> * Check for entries in extras that are within the specified
> * directory, where dirname is a reference directory name including
>
^ permalink raw reply
* Re: [PATCH] squash! completion: fill COMPREPLY directly when completing refs
From: Junio C Hamano @ 2017-02-13 20:24 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Git mailing list
In-Reply-To: <CAM0VKj=Pai0fL7KtMdv1sg3N2KA1aBafGQ_XzXWKUsBmo62ZoA@mail.gmail.com>
SZEDER Gábor <szeder.dev@gmail.com> writes:
> On Fri, Feb 10, 2017 at 10:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Should I expect a reroll to come, or is this the only fix-up to the
>> series that begins at <20170203025405.8242-1-szeder.dev@gmail.com>?
>>
>> No hurries.
>
> Yes, definitely.
>
> I found a minor bug in the middle of the series, and haven't quite
> made up my mind about it and its fix yet. Perhaps in a day or three.
>
> Regarding the 'strip' -> 'lstrip' 'for-each-ref' format modifier
> rename that broke pu: I should keep using 'strip', right?
Right. I view the removal of 'strip' as an accident when 'lstrip'
was introduced, not an intended change.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox