* Re: [PATCH v3 2/5] stash: introduce push verb
From: Thomas Gummerer @ 2017-02-13 22:16 UTC (permalink / raw)
To: Matthieu Moy
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz, Jeff King,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <vpqlgtaz09q.fsf@anie.imag.fr>
On 02/13, Matthieu Moy wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > Introduce a new git stash push verb in addition to git stash save. The
> > push verb is used to transition from the current command line arguments
> > to a more conventional way, in which the message is given as an argument
> > to the -m option.
>
> Sorry if this has been discussed before, but I find 'push' rather
> confusing here. It took me a while to understand that it meant "opposite
> of pop", because in the context of Git, "push" usually means "send to
> remote repository".
There wasn't much of a discussion about it, but it was pretty much the
only thing that came to my mind, and nobody complained or suggested
anything different, so I just went with it. No other verb came to my
mind yet, but if someone has a better suggestion, I'd be happy to
change.
> Unfortunately, I didn't come up with a better name. "create" is already
> taken ...
>
> Another think to have in mind: changing the option name to break
> backward compatibility is something we can't do often, so if there's
> anything else we should change about the UI, we should do it now. I
> don't have anything particular in mind, just thinking aloud.
Now that you mention this, there actually is one inconsistency that I
introduced, which I shouldn't have. git stash push works with
--include-untracked and --all to decide whether or not to include
untracked files, and if also ignored files should be included. I also
added a --include-untracked {untracked,all} argument to git stash
create, which is clearly inconsistent.
There really should only be one way. I'd be fine with either way, but
I think using --include-untracked and --all is the better choice,
because it's easy to understand, and also makes it easier to switch
git stash without a verb over to use push_stash internally.
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH] clean: use warning_errno() when appropriate
From: Jeff King @ 2017-02-13 22:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <xmqqy3x9bvvm.fsf@gitster.mtv.corp.google.com>
On Mon, Feb 13, 2017 at 01:53:33PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > IOW, I think this may be a case where we should be optimizing for
> > programmer time (fewer lines of code, and one less thing to worry about
> > in the callers) versus squeezing out every instruction.
>
> Fair enough.
>
> Unless we do the save_errno dance in all the helper functions we
> commonly use to safely stash away errno as necessary and tell
> developers that they can depend on it, the code in the patch that
> began this discussion still needs its own saved_errno dance to be
> safe, though. I do not have a feeling that we are not there yet,
> even after we teach xmalloc() and its family to do so.
Yeah, I certainly agree that is a potential blocker. Even if it is true
today, there's nothing guaranteeing that the quote functions don't grow
a new internal detail that violates.
So in that sense doing the errno dance as close to the caller who cares
is the most _obvious_ thing, even if it isn't the shortest.
It would be nice if there was a way to annotate a function as
errno-safe, and then transitively compute which other functions were
errno-safe when they do not call any errno-unsafe function. I don't know
if any static analyzers allow that kind of custom annotation, though
(and also I wonder whether the cost/benefit of maintaining those
annotations would be worth it).
-Peff
^ permalink raw reply
* Re: [PATCH v3 0/5] stash: support pathspec argument
From: Thomas Gummerer @ 2017-02-13 22:33 UTC (permalink / raw)
To: Jeff King
Cc: Matthieu Moy, git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <20170213214521.pkjesijdlus36tnp@sigill.intra.peff.net>
On 02/13, Jeff King wrote:
> On Mon, Feb 13, 2017 at 10:35:31PM +0100, Matthieu Moy wrote:
>
> > > 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.
> >
> > Indeed -p is not the best example. In the old thread, I used -q which is
> > much more problematic:
> >
> > git stash -q drop => interpreted as: git stash push -q drop
> > git stash drop -q => drop with option -q
>
> Yeah, I'd agree with that. I wouldn't propose to loosen it entirely, but
> rather to treat "-p" specially.
>
> > It's not really "dangerous" at least in this case, since we misinterpret
> > a destructive command for a less destructive one, but it is rather
> > confusing that changing the order between command and options change the
> > behavior.
> >
> > I actually find it a reasonable expectation to allow swapping commands
> > and options, some programs other than git allow it.
>
> I think we may have already crossed that bridge with "git -p stash".
>
> Not to mention that the ordering already _is_ relevant (we disallow one
> order but not the other). If we really wanted to allow swapping, it
> would mean making:
>
> git stash -p drop
>
> the same as:
>
> git stash drop -p
>
> I actually find _that_ more confusing. It would perhaps make more sense
> with something like "-q", which is more of a "global" option than a
> command-specific one. But I think we'd want to whitelist such global
> options (and "-p" would not be on that list).
>
> > > 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.
> >
> > Ah, OK. But that's not really hard to implement: when going through the
> > option list looking for non-option, shift one more time when finding -m.
>
> No, it's not hard conceptually. It just means implementing the
> option-parsing policy in two places. That's not too bad now, but if we
> started using rev-parse's options helper, then I think you have corner
> cases like "git stash -km foo".
>
> My "-p" suggestion suffers from a similar problem if you treat it as
> "you can omit the 'push' if you say "-p", rather than "if -p is the
> first option, it is a synonym for 'push -p'".
I'm almost convinced of special casing "-p". (Maybe I'm easy to
convince as well, because it would be convenient ;) ) However it's a
bit weird that now "git stash -p file" would work, but "git stash -m
message" wouldn't. Maybe we should do it the other way around, and
only special case "-q", and see if there is an non option argument
after that? From a glance at the options that's the only one where
"git stash -<option> <verb>" could make sense to the user.
^ permalink raw reply
* [PATCH] mingw: make stderr unbuffered again
From: Johannes Schindelin @ 2017-02-13 22:34 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Sixt, Jeff Hostetler
When removing the hack for isatty(), we actually removed more than just
an isatty() hack: we removed the hack where internal data structures of
the MSVC runtime are modified in order to redirect stdout/stderr.
Instead of using that hack (that does not work with newer versions of
the runtime, anyway), we replaced it by reopening the respective file
descriptors.
What we forgot was to mark stderr as unbuffered again.
Reported by Hannes Sixt. Fixed with Jeff Hostetler's assistance.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/mingw-unbuffered-stderr-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-unbuffered-stderr-v1
compat/winansi.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/compat/winansi.c b/compat/winansi.c
index 82b89ab1376..793420f9d0d 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -510,6 +510,8 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
*/
close(new_fd);
+ if (fd == 2)
+ setvbuf(stderr, NULL, _IONBF, BUFSIZ);
fd_is_interactive[fd] |= FD_SWAPPED;
return duplicate;
@@ -547,6 +549,8 @@ static void detect_msys_tty(int fd)
!wcsstr(name, L"-pty"))
return;
+ if (fd == 2)
+ setvbuf(stderr, NULL, _IONBF, BUFSIZ);
fd_is_interactive[fd] |= FD_MSYS;
}
base-commit: 5588dbffbd61e4906e453808c6ad32f792fea521
--
2.11.1.windows.1
^ permalink raw reply related
* Re: [PATCH 06/11] refs-internal.h: correct is_per_worktree_ref()
From: Stefan Beller @ 2017-02-13 22:37 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git@vger.kernel.org, Junio C Hamano, Michael Haggerty,
Johannes Schindelin, David Turner
In-Reply-To: <20170213152011.12050-7-pclouds@gmail.com>
On Mon, Feb 13, 2017 at 7:20 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> All refs outside refs/ directory is per-worktree, not just HEAD.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> refs/refs-internal.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index f4aed49f5..69d02b6ba 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -653,7 +653,7 @@ const char *resolve_ref_recursively(struct ref_store *refs,
>
> static inline int is_per_worktree_ref(const char *refname)
> {
> - return !strcmp(refname, "HEAD") ||
> + return !starts_with(refname, "refs/") ||
> starts_with(refname, "refs/bisect/");
you're loosing HEAD here? (assuming we get HEAD in
short form here, as well as long form refs/HEAD)
> }
>
> --
> 2.11.0.157.gd943d85
>
^ permalink raw reply
* Re: [RFC PATCH] show decorations at the end of the line
From: Jeff King @ 2017-02-13 22:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Git Mailing List
In-Reply-To: <xmqq7f4tdcua.fsf@gitster.mtv.corp.google.com>
On Mon, Feb 13, 2017 at 01:01:49PM -0800, Junio C Hamano wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
> > 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).
>
> OK. So let's wait to hear from others if they like the "obviously"
> improved output. Even though I find the decorations indispensable
> in my "git log" output, I personally do not have much preference
> either way, as my screen is often wide enough ;-)
I have a slight preference for the new output (decorations at the end)
versus the original, but I could go either way.
I don't think the scripting compatibility concerns are an issue, for all
the reasons given in the thread.
There is one related option, --source, which also puts its data between
the hash and the subject in --oneline. In theory that should be treated
similarly, though:
1. It's already really ugly, as it does not even get the parentheses
and coloring.
2. It's perhaps more likely to get scripted, as it really is parseable
in the current state.
I'm not sure if a better path forward would be to just extend the idea
of "decorator" to possibly include more than just ref-tips. On the other
hand, if you really want to get fancy with formatting, we already have a
complete formatting language. Perhaps it should learn a placeholder for
the "--source" data.
Similarly, I've often wanted a "contained in this tags/branches"
annotation for each commit. It's not too expensive to compute if you
topo-sort the set of commits and just paint down as you traverse.
Anyway, I think none of that needs to block changes to --decorate
output. Just thinking out loud.
-Peff
^ permalink raw reply
* Re: [PATCH] mingw: use OpenSSL's SHA-1 routines
From: Johannes Schindelin @ 2017-02-13 22:38 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Jeff King, git, Jeff Hostetler
In-Reply-To: <b530c820-9956-4396-d853-c7d70ccaf11d@kdbg.org>
Hi,
On Mon, 13 Feb 2017, Johannes Sixt wrote:
> Am 13.02.2017 um 20:42 schrieb Junio C Hamano:
> > 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?
>
> Your first assumption is incorrect as far as I am concerned. I build
> from your tree plus some topics. During -rc period, I build off of
> master; after a release, I build off of next. I merge some of the topics
> that you carry in pu when I find them interesting or when I suspect them
> to regress on Windows. Then I carry around a few additional patches
> that the public has never seen, and these days I also merge Dscho's
> rebase-i topic.
In addition, you build from a custom MINGW/MSys1 setup, correct?
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] mingw: make stderr unbuffered again
From: Junio C Hamano @ 2017-02-13 22:39 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Johannes Sixt, Jeff Hostetler
In-Reply-To: <c88612da0a62bfcbc3e278296f9d3eb010057071.1487025228.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> When removing the hack for isatty(), we actually removed more than just
> an isatty() hack: we removed the hack where internal data structures of
> the MSVC runtime are modified in order to redirect stdout/stderr.
>
> Instead of using that hack (that does not work with newer versions of
> the runtime, anyway), we replaced it by reopening the respective file
> descriptors.
>
> What we forgot was to mark stderr as unbuffered again.
>
> Reported by Hannes Sixt. Fixed with Jeff Hostetler's assistance.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/mingw-unbuffered-stderr-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git mingw-unbuffered-stderr-v1
OK. Should this go directly to 'master', as the isatty thing is
already in?
>
> compat/winansi.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/compat/winansi.c b/compat/winansi.c
> index 82b89ab1376..793420f9d0d 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -510,6 +510,8 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
> */
> close(new_fd);
>
> + if (fd == 2)
> + setvbuf(stderr, NULL, _IONBF, BUFSIZ);
> fd_is_interactive[fd] |= FD_SWAPPED;
>
> return duplicate;
> @@ -547,6 +549,8 @@ static void detect_msys_tty(int fd)
> !wcsstr(name, L"-pty"))
> return;
>
> + if (fd == 2)
> + setvbuf(stderr, NULL, _IONBF, BUFSIZ);
> fd_is_interactive[fd] |= FD_MSYS;
> }
>
>
> base-commit: 5588dbffbd61e4906e453808c6ad32f792fea521
^ permalink raw reply
* Re: [PATCH v3 4/5] stash: introduce new format create
From: Jeff King @ 2017-02-13 23:05 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <20170213215734.puoung6hhdifbgai@sigill.intra.peff.net>
On Mon, Feb 13, 2017 at 04:57:34PM -0500, Jeff King wrote:
> Yeah, I think your patch is actually fixing that case. But your search
> is only part of the story. You found somebody using "-m" explicitly, but
> what about somebody blindly calling:
>
> git stash create $*
>
> That's now surprising to somebody who puts "-m" in their message.
>
> > I *think* this regression is acceptable, but I'm happy to introduce
> > another verb if people think otherwise.
>
> Despite what I wrote above, I'm still inclined to say that this isn't an
> important regression. I'd be surprised if "stash create" is used
> independently much at all.
Just thinking on this more...do we really care about "fixing" the
interface of "stash create"? This is really just about refactoring what
underlies the new "push", right?
So we could just do:
diff --git a/git-stash.sh b/git-stash.sh
index 6d629fc43..ee37db135 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -711,7 +711,7 @@ clear)
;;
create)
shift
- create_stash "$@" && echo "$w_commit"
+ create_stash -m "$*" && echo "$w_commit"
;;
store)
shift
on top of your patch and keep the external interface the same.
It might be nice to clean up the interface for "create" to match other
ones, but from this discussion I think it is mostly a historical wart
for scripting, and we are OK to just leave its slightly-broken interface
in place forever.
I could go either way.
-Peff
^ permalink raw reply related
* Re: [PATCH 07/11] files-backend: remove the use of git_path()
From: Stefan Beller @ 2017-02-13 23:09 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git@vger.kernel.org, Junio C Hamano, Michael Haggerty,
Johannes Schindelin, David Turner
In-Reply-To: <20170213152011.12050-8-pclouds@gmail.com>
> +
> + if (submodule) {
> + refs->submodule = xstrdup_or_null(submodule);
drop the _or_null here?
So in this patch we have either
* submodule set
* or gitdir/gitcommondir set
which means that we exercise the commondir for regular repos.
In the future when we want to be able to have a combination of worktrees
and submodules this ought to be possible by setting submodule != NULL
and still populating the gitdir/commondir buffers.
Thanks,
Stefan
^ permalink raw reply
* new git-diff switch to eliminate leading "+" and "-" characters
From: Vanderhoof, Tzadik @ 2017-02-13 23:01 UTC (permalink / raw)
To: git@vger.kernel.org
The output of git-diff includes lines beginning with "+" and "-" to indicate added and deleted lines. A somewhat common task (at least for me) is to want to copy output from a "diff" (usually the deleted lines) and paste it back into my code.
This is quite inconvenient because of the leading "+" and "-" characters. I know there are shell and IDE / editor workarounds but it would be nice if there was a switch to git-diff to make it leave out those characters, especially since "--color" kind of makes those leading characters obsolete.
Would it make sense to develop such a switch or has there been work on that already?
______________________________
Tzadik Vanderhoof | Optum360
Sr Software Engineer, NLP Innovation
www.optum360.com
This e-mail, including attachments, may include confidential and/or
proprietary information, and may be used only by the person or entity
to which it is addressed. If the reader of this e-mail is not the intended
recipient or his or her authorized agent, the reader is hereby notified
that any dissemination, distribution or copying of this e-mail is
prohibited. If you have received this e-mail in error, please notify the
sender by replying to this message and delete this e-mail immediately.
^ permalink raw reply
* Re: [PATCH 08/11] refs.c: factor submodule code out of get_ref_store()
From: Stefan Beller @ 2017-02-13 23:13 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git@vger.kernel.org, Junio C Hamano, Michael Haggerty,
Johannes Schindelin, David Turner
In-Reply-To: <20170213152011.12050-9-pclouds@gmail.com>
On Mon, Feb 13, 2017 at 7:20 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> 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>
> ---
Looks good,
Thanks,
Stefan
^ permalink raw reply
* Developing git with IDE
From: Oleg Taranenko @ 2017-02-13 23:15 UTC (permalink / raw)
To: git
Hi *,
being last decade working with java & javascript I completely lost
relation to c/c++ world. Trying to get into git internals I'm facing
with issue what IDE is more suitable for developing git @ macos ?
Have googled, but any my search queries following to non-relevant
themes, like supporting git in IDEs etc.
my first attempt - CLion (as far as I'm Jetbrains fan) - got failed,
as far as doesn't support makefile-based projects, only CMake.
There are a number of free C/C++ dev tools: Xcode, CodeBlocks,
CodeLite. Gnat, Qt creator, Dev C++, C++ Builder (Borland? :),
Eclipse, NetBeans... what else?
Because of lack my modern C experience, could somebody share his own
attempts/thoughts/use cases how more convenient dive into the git
development process on the Mac?
Tried to find in the git distribution Documentation more information
about this, nothing as well... Would be nice to have a kind of
'Getting Started Manual'
Thanks for your time,
Oleg Taranenko
^ permalink raw reply
* Re: [PATCH v2 9/9] read_loose_refs(): read refs using resolve_ref_recursively()
From: Junio C Hamano @ 2017-02-13 23:18 UTC (permalink / raw)
To: Michael Haggerty
Cc: Nguyễn Thái Ngọc Duy, Stefan Beller,
Johannes Schindelin, David Turner, Jeff King, git
In-Reply-To: <ff0b0df6-9aed-9417-d9d4-1234d53f05c3@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> I pushed the fixed commit to branch `submodule-hash` in my fork [1]. If
> you'd like me to send it to the mailing list again, please let me know.
I was tempted to ask you to send it again, because fetching,
comparing and then cherry-picking is a lot more work than just
replacing one, but just to make sure my intuition about the
work required is correct, I did it anyway and yes, fetching,
comparing, cherry-picking and then amending the sign-off in was a
lot more work ;-)
So no need to resend. Thanks.
^ permalink raw reply
* Re: [PATCH v6] gc: ignore old gc.log files
From: Junio C Hamano @ 2017-02-13 23:21 UTC (permalink / raw)
To: David Turner; +Cc: git, peff, pclouds
In-Reply-To: <20170210212822.14988-1-dturner@twosigma.com>
David Turner <dturner@twosigma.com> writes:
> +static unsigned long gc_log_expire_time;
> +static const char *gc_log_expire = "1.day.ago";
OK.
> @@ -113,6 +133,8 @@ static void gc_config(void)
> git_config_get_bool("gc.autodetach", &detach_auto);
> git_config_date_string("gc.pruneexpire", &prune_expire);
> git_config_date_string("gc.worktreepruneexpire", &prune_worktrees_expire);
> + git_config_date_string("gc.logexpiry", &gc_log_expire);
> +
OK.
I think I had a stale one queued; will replace.
Thanks.
^ permalink raw reply
* [PATCH] completion: complete modified files for checkout with '--'
From: cornelius.weig @ 2017-02-13 23:33 UTC (permalink / raw)
To: git; +Cc: Cornelius Weig, szeder.dev, j6t, bitte.keine.werbung.einwerfen
From: Cornelius Weig <cornelius.weig@tngtech.com>
The command line completion for git-checkout bails out when seeing '--'
as an isolated argument. For git-checkout this signifies the start of a
list of files which are to be checked out. Checkout of files makes only
sense for modified files, therefore completion can be a bit smarter:
Instead of bailing out, offer modified files for completion.
Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---
contrib/completion/git-completion.bash | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6c6e1c7..d6523fd 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1059,7 +1059,10 @@ _git_bundle ()
_git_checkout ()
{
- __git_has_doubledash && return
+ __git_has_doubledash && {
+ __git_complete_index_file "--modified"
+ return
+ }
case "$cur" in
--conflict=*)
--
2.10.2
^ permalink raw reply related
* Re: [PATCH 09/11] refs: move submodule code out of files-backend.c
From: Stefan Beller @ 2017-02-13 23:35 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git@vger.kernel.org, Junio C Hamano, Michael Haggerty,
Johannes Schindelin, David Turner
In-Reply-To: <20170213152011.12050-10-pclouds@gmail.com>
On Mon, Feb 13, 2017 at 7:20 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> files-backend is now initialized with a $GIT_DIR. Converting a submodule
> path to where real submodule gitdir is located is done in get_ref_store().
>
> The new code in init_submodule_ref_store() is basically a copy of
> strbuf_git_path_submodule().
>
> This gives a slight performance improvement for submodules since we
> don't convert submodule path to gitdir at every backend call like
> before. We pay that once at ref-store creation.
>
> More cleanup in files_downcast() follows shortly. It's separate to keep
> noises from this patch.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> refs.c | 48 +++++++++++++++++++++++++++++++++++++++++-------
> refs/files-backend.c | 25 +++++++------------------
> refs/refs-internal.h | 6 +++---
> 3 files changed, 51 insertions(+), 28 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 8ef7a52ba..9ac194945 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -9,6 +9,7 @@
> #include "refs/refs-internal.h"
> #include "object.h"
> #include "tag.h"
> +#include "submodule-config.h"
>
> /*
> * List of all available backends
> @@ -1410,7 +1411,7 @@ static void register_ref_store(struct ref_store *refs, const char *submodule)
> * Create, record, and return a ref_store instance for the specified
> * submodule (or the main repository if submodule is NULL).
> */
> -static struct ref_store *ref_store_init(const char *submodule)
> +static struct ref_store *ref_store_init(const char *submodule, const char *gitdir)
> {
> const char *be_name = "files";
> struct ref_storage_be *be = find_ref_storage_backend(be_name);
> @@ -1419,7 +1420,7 @@ static struct ref_store *ref_store_init(const char *submodule)
> if (!be)
> die("BUG: reference backend %s is unknown", be_name);
>
> - refs = be->init(submodule);
> + refs = be->init(gitdir);
> register_ref_store(refs, submodule);
> return refs;
> }
> @@ -1445,15 +1446,48 @@ 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)
> +static struct ref_store *init_submodule_ref_store(const char *path)
> {
> struct strbuf submodule_sb = STRBUF_INIT;
> + struct strbuf git_submodule_common_dir = STRBUF_INIT;
> + struct strbuf git_submodule_dir = STRBUF_INIT;
> + struct strbuf buf = STRBUF_INIT;
> + const char *git_dir;
> + const struct submodule *sub;
> struct ref_store *refs = NULL;
>
> - strbuf_addstr(&submodule_sb, submodule);
> - if (is_nonbare_repository_dir(&submodule_sb))
> - refs = ref_store_init(submodule);
> + strbuf_addstr(&submodule_sb, path);
> + if (!is_nonbare_repository_dir(&submodule_sb))
> + goto done;
> +
> + strbuf_addstr(&buf, path);
> + strbuf_complete(&buf, '/');
> + strbuf_addstr(&buf, ".git");
> +
> + git_dir = read_gitfile(buf.buf);
if buf.buf is a (git) directory as opposed to a git file,
we error out in read_gitfile. Did you mean to use
read_gitfile_gently here or rather even resolve_gitdir_gently ?
> + if (git_dir) {
when not using the _gently version git_dir is always
non NULL here (or we're dead)?
> + strbuf_reset(&buf);
> + strbuf_addstr(&buf, git_dir);
> + }
> + if (!is_git_directory(buf.buf)) {
> + gitmodules_config();
> + sub = submodule_from_path(null_sha1, path);
> + if (!sub)
> + goto done;
> + strbuf_reset(&buf);
> + strbuf_git_path(&buf, "%s/%s", "modules", sub->name);
You can inline "modules" into the format string?
> + }
> + strbuf_addch(&buf, '/');
> + strbuf_addbuf(&git_submodule_dir, &buf);
> +
> + refs = ref_store_init(path, git_submodule_dir.buf);
strbuf_detach (git_submodule_dir) here, such that we keep
the string alive despite the release of the strbuf below?
so essentially this function
* takes a submodule path
* checks if there is a repo at the given path in the working tree
* resolves the gitfile if any
* if the gitfile could not resolve to a valid repo just make up the
location to be $GIT_DIR/modules/<name>
sounds confusing to me. I need to reread it later.
>
> - if (submodule) {
> - refs->submodule = xstrdup_or_null(submodule);
> + if (gitdir) {
> + strbuf_addstr(&refs->gitdir, gitdir);
> + get_common_dir_noenv(&refs->gitcommondir, gitdir);
Oh I see. we loose the _or_null here, so my remark on the previous patch
might be just unneeded work.
> } else {
> strbuf_addstr(&refs->gitdir, get_git_dir());
> strbuf_addstr(&refs->gitcommondir, get_git_common_dir());
> @@ -1034,8 +1025,6 @@ static struct ref_store *files_ref_store_create(const char *submodule)
> static void files_assert_main_repository(struct files_ref_store *refs,
> const char *caller)
> {
> - if (refs->submodule)
> - die("BUG: %s called for a submodule", caller);
> }
In a followup we'd get rid of files_assert_main_repository
presumably?
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH 10/11] files-backend: remove submodule_allowed from files_downcast()
From: Stefan Beller @ 2017-02-13 23:44 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git@vger.kernel.org, Junio C Hamano, Michael Haggerty,
Johannes Schindelin, David Turner
In-Reply-To: <20170213152011.12050-11-pclouds@gmail.com>
On Mon, Feb 13, 2017 at 7:20 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> Since submodule or not is irrelevant to files-backend after the last
> patch, remove the parameter from files_downcast() and kill
> files_assert_main_repository().
>
> PS. This implies that all ref operations are allowed for submodules. But
> we may need to look more closely to see if that's really true...
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
Looks good,
Stefan
^ permalink raw reply
* Re: Continuous Testing of Git on Windows
From: Junio C Hamano @ 2017-02-13 23:46 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git-for-windows, git
In-Reply-To: <alpine.DEB.2.20.1702101241210.3496@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> That is why I taught the Git for Windows CI job that tests the four
> upstream Git integration branches to *also* bisect test breakages and then
> upload comments to the identified commit on GitHub
Good. I do not think it is useful to try 'pu' as an aggregate and
expect it to always build and work [*1*], but your "bisect and
pinpoint" approach makes it useful to identify individual topic that
brings in a breakage. I wouldn't be surprised if original submitter
and I were the only two people who actually compiled the patches on
a topic in isolation while a topic is in 'pu', and chances are that
these two people didn't try their builds on Windows. A CI like this
one will help the coverage to stop premature topics from advancing
to 'pu' without getting any Windows exposure.
Thanks.
[Footnote]
*1* The reason why topics not in 'next' but in 'pu', especially the
ones merged near the tip of 'pu', exist in 'pu' are because they
are interesting enough and could be polished to become eligible
for 'next' but known to be premature for 'next' yet. They are
there primarily to give human contributors an easier way to
download them as a whole and help polish them. And I have to be
selective when I queue things on 'pu'; it is not like I have
infinite amount of time to pick up any cruft that is sent to the
list.
^ permalink raw reply
* Re: [PATCH] clean: use warning_errno() when appropriate
From: Junio C Hamano @ 2017-02-13 23:48 UTC (permalink / raw)
To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <20170213222210.dtfnxbbi77tsx4a6@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> So in that sense doing the errno dance as close to the caller who cares
> is the most _obvious_ thing, even if it isn't the shortest.
Yup.
> It would be nice if there was a way to annotate a function as
> errno-safe, and then transitively compute which other functions were
> errno-safe when they do not call any errno-unsafe function. I don't know
> if any static analyzers allow that kind of custom annotation, though
> (and also I wonder whether the cost/benefit of maintaining those
> annotations would be worth it).
Double yup.
^ permalink raw reply
* Re: [PATCH 11/11] refs: split and make get_*_ref_store() public API
From: Stefan Beller @ 2017-02-13 23:55 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git@vger.kernel.org, Junio C Hamano, Michael Haggerty,
Johannes Schindelin, David Turner
In-Reply-To: <20170213152011.12050-12-pclouds@gmail.com>
>
> +/*
> + * Return the ref_store instance for the specified submodule. For the
> + * main repository, use submodule==NULL; such a call cannot fail.
So now we have both a get_main as well as a get_submodule function,
but the submodule function can return the main as well?
I'd rather see this as a BUG; or asking another way:
What is the difference between get_submodule_ref_store(NULL)
and get_main_ref_store() ?
As you went through all call sites (by renaming the function), we'd
be able to tell that there is no caller with NULL, or is it?
Stefan
> 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);
^ permalink raw reply
* [PATCH for NEXT] grep: do not unnecessarily query repo for "--"
From: Jonathan Tan @ 2017-02-14 0:11 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, peff, gitster
When running a command of the form
git grep --no-index pattern -- path
in the absence of a Git repository, an error message will be printed:
fatal: BUG: setup_git_env called without repository
This is because "git grep" tries to interpret "--" as a rev. "git grep"
has always tried to first interpret "--" as a rev for at least a few
years, but this issue was upgraded from a pessimization to a bug in
commit 59332d1 ("Resurrect "git grep --no-index"", 2010-02-06), which
calls get_sha1 regardless of whether --no-index was specified. This bug
appeared to be benign until commit b1ef400 ("setup_git_env: avoid blind
fall-back to ".git"", 2016-10-20) when Git was taught to die in this
situation. (This "git grep" bug appears to be one of the bugs that
commit b1ef400 is meant to flush out.)
Therefore, always interpret "--" as signaling the end of options,
instead of trying to interpret it as a rev first.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
There is probably a similar bug for commands of the form:
git grep --no-index pattern foo
If there is a repo and "foo" is a rev, the "--no-index or --untracked
cannot be used with revs." error would occur. If there is a repo and
"foo" is not a rev, this command would proceed as usual. If there is no
repo, the "setup_git_env called without repository" error would occur.
(This is my understanding from reading the code - I haven't tested it
out.)
This patch does not fix this similar bug, but I decided to send it out
anyway because it still fixes a bug and unlocks the ability to
specify paths with "git grep --no-index".
builtin/grep.c | 9 +++++----
t/t7810-grep.sh | 12 ++++++++++++
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 2c727ef49..1b68d1638 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1154,6 +1154,11 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
const char *arg = argv[i];
unsigned char sha1[20];
struct object_context oc;
+ if (!strcmp(arg, "--")) {
+ i++;
+ seen_dashdash = 1;
+ break;
+ }
/* Is it a rev? */
if (!get_sha1_with_context(arg, 0, sha1, &oc)) {
struct object *object = parse_object_or_die(sha1, arg);
@@ -1162,10 +1167,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
add_object_array_with_path(object, arg, &list, oc.mode, oc.path);
continue;
}
- if (!strcmp(arg, "--")) {
- i++;
- seen_dashdash = 1;
- }
break;
}
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 19f0108f8..29202f0e7 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -982,6 +982,18 @@ test_expect_success 'grep -e -- -- path' '
test_cmp expected actual
'
+test_expect_success 'grep --no-index pattern -- path' '
+ rm -fr non &&
+ mkdir -p non/git &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ echo hello >hello &&
+ git grep --no-index o -- .
+ )
+'
+
cat >expected <<EOF
hello.c:int main(int argc, const char **argv)
hello.c: printf("Hello world.\n");
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* Re: [PATCH v3 0/5] stash: support pathspec argument
From: Jeff King @ 2017-02-14 0:27 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Gummerer, Matthieu Moy, git, Stephan Beyer, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <xmqqwpctabvw.fsf@gitster.mtv.corp.google.com>
On Mon, Feb 13, 2017 at 03:50:43PM -0800, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> >> My "-p" suggestion suffers from a similar problem if you treat it as
> >> "you can omit the 'push' if you say "-p", rather than "if -p is the
> >> first option, it is a synonym for 'push -p'".
> >
> > I'm almost convinced of special casing "-p". (Maybe I'm easy to
> > convince as well, because it would be convenient ;) ) However it's a
> > bit weird that now "git stash -p file" would work, but "git stash -m
> > message" wouldn't.
>
> I am not sure why this matters. The original "git stash <msg>" was
> just "Are you being extremely busy and cannot even afford to type
> 'save'? Ok, let me assume you meant that!". Now we are talking
> about picking and choosing hunks carefully going through interactive
> process, I really do not think there is any justification to infer
> 'push' when 'push' was omitted in "git stash push -p" the user wants
> to do.
Maybe it is just me and my muscle memory, but "git stash -p" is quite a
common command for me[1]. And I have typed "git stash -p foo" many times
and been annoyed that it didn't work. I was hoping to end that
annoyance.
I guess I could make an alias and retrain my fingers.
-Peff
[1] I almost never run "reset --hard", preferring instead to stash away
changes just in case I would change my mind later and want them. And
I quite often use "stash -p" because I like to double check what I
am throwing away.
I also use "stash -p" heavily when picking apart changes from the
working tree.
^ permalink raw reply
* Re: [PATCH v3 0/5] stash: support pathspec argument
From: Jeff King @ 2017-02-14 0:37 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Gummerer, Matthieu Moy, git, Stephan Beyer, Marc Strapetz,
Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <xmqqpoila9rt.fsf@gitster.mtv.corp.google.com>
On Mon, Feb 13, 2017 at 04:36:22PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Maybe it is just me and my muscle memory, but "git stash -p" is quite a
> > common command for me[1]. And I have typed "git stash -p foo" many times
> > and been annoyed that it didn't work. I was hoping to end that
> > annoyance.
>
> OK, I never run "stash -p" and did not realize people already find
> it useful that it becomes "stash save -p". Please ignore me, then.
> I agree that breaking the established use is not nice.
To be fair, I don't think anybody is proposing breaking the established
use of "stash -p". I was just hoping the new pathspec feature could
trickle down to it, as well. :)
-Peff
^ permalink raw reply
* Re: [RFC-PATCHv2] submodules: add a background story
From: Brandon Williams @ 2017-02-14 0:39 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
In-Reply-To: <20170209020855.23486-1-sbeller@google.com>
On 02/08, Stefan Beller wrote:
> +STATES
> +------
> +
> +When working with submodules, you can think of them as in a state machine.
> +So each submodule can be in a different state, the following indicators are used:
> +
> +* the existence of the setting of 'submodule.<name>.url' in the
> + superprojects configuration
> +* the existence of the submodules working tree within the
> + working tree of the superproject
> +* the existence of the submodules git directory within the superprojects
> + git directory at $GIT_DIR/modules/<name> or within the submodules working
> + tree
> +
> + State URL config working tree git dir
> + -----------------------------------------------------
> + uninitialized no no no
> + initialized yes no no
> + populated yes yes yes
> + depopulated yes no yes
> + deinitialized no no yes
> + uninteresting no yes yes
> +
> + invalid no yes no
> + invalid yes yes no
> + -----------------------------------------------------
> +
> +The first six states can be reached by normal git usage, the latter two are
> +only shown for completeness to show all possible eight states with 3 binary
> +indicators. The states in detail:
> +
> +uninitialized::
> +The uninitialized state is the default state if no
> +'--recurse-submodules' / '--recursive'. An empty directory will be put in
> +the working tree as a place holder, such that you are reminded of the
> +existence of the submodule.
> +---
> +To transition into the initialized state
> +you can use 'git submodule init', which copies the presets from the
> +.gitmodules file into the config.
> +
> +initialized::
> +Users transitioned from the uninitialized state to this state via
> +'git submodule init', which preset the URL configuration. As these URLs
> +may not be desired in certain scenarios, this state allows to change the
> +URLs. For example in a corporate environment you may want to run
> +
> + sed -i s/example.org/$internal-mirror/ .git/config
> ++
Maybe we can try to brainstorm and come up with some clearer terminology
while we are at it. I was trying to think about the "initialized" state
and I may be the only one but it seems unclear what "initialized" means.
I mean I already have all the information about a submodule in the
.gitmodules file, isn't it already initialized then? Maybe this state
would be better named "(in)active" as a module that is interesting to a
user is "active"?
--
Brandon Williams
^ 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