Re: git fetch behaves weirdely when run in a worktree
Hi, I just wanted make a point a little more clear. See comment inline. On 24 September 2018 01:39:26 GMT+05:30, Kaartic Sivaraam wrote: > To add to that >confusion when I run > > $ cat $MAIN_WORKTREE/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD > >it seems to be printing the info about the remote refs once and then if >I run it again immediately nothing is printed. If I repeat it again, >info about remote refs is printed but this time the info about the >remote refs is printed thrice. This happens randomly. Sample output: > > 01:23 $ cat ../git/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD >53f9a3e157dbbc901a02ac2c73346d375e24978c not-for-merge branch 'maint' >of https://github.com/git/git >150f307afc13961b0322ad0e7205a7b193368586 not-for-merge branch 'master' >of https://github.com/git/git >01d371f741e6f0b0076d9ed942d99bdb23757eac not-for-merge branch 'next' of >https://github.com/git/git >7a81cbc028be113058e0b55062706ec6de62ed94 not-for-merge branch 'pu' of >https://github.com/git/git >722746685bce03f223ed75febe312495e6c139da not-for-merge branch 'todo' of >https://github.com/git/git > 01:23 $ cat ../git/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD > 01:23 $ cat ../git/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD > 01:23 $ cat ../git/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD >53f9a3e157dbbc901a02ac2c73346d375e24978c not-for-merge branch 'maint' >of https://github.com/git/git >150f307afc13961b0322ad0e7205a7b193368586 not-for-merge branch 'master' >of https://github.com/git/git >01d371f741e6f0b0076d9ed942d99bdb23757eac not-for-merge branch 'next' of >https://github.com/git/git >7a81cbc028be113058e0b55062706ec6de62ed94 not-for-merge branch 'pu' of >https://github.com/git/git >722746685bce03f223ed75febe312495e6c139da not-for-merge branch 'todo' of >https://github.com/git/git >53f9a3e157dbbc901a02ac2c73346d375e24978c not-for-merge branch 'maint' >of https://github.com/git/git >150f307afc13961b0322ad0e7205a7b193368586 not-for-merge branch 'master' >of https://github.com/git/git >01d371f741e6f0b0076d9ed942d99bdb23757eac not-for-merge branch 'next' of >https://github.com/git/git >7a81cbc028be113058e0b55062706ec6de62ed94 not-for-merge branch 'pu' of >https://github.com/git/git >722746685bce03f223ed75febe312495e6c139da not-for-merge branch 'todo' of >https://github.com/git/git >53f9a3e157dbbc901a02ac2c73346d375e24978c not-for-merge branch 'maint' >of https://github.com/git/git >150f307afc13961b0322ad0e7205a7b193368586 not-for-merge branch 'master' >of https://github.com/git/git >01d371f741e6f0b0076d9ed942d99bdb23757eac not-for-merge branch 'next' of >https://github.com/git/git >7a81cbc028be113058e0b55062706ec6de62ed94 not-for-merge branch 'pu' of >https://github.com/git/git >722746685bce03f223ed75febe312495e6c139da not-for-merge branch 'todo' of >https://github.com/git/git > 01:23 $ cat ../git/.git/worktrees/$BUILD_WORKTREE/FETCH_HEAD >53f9a3e157dbbc901a02ac2c73346d375e24978c not-for-merge branch 'maint' >of https://github.com/git/git >150f307afc13961b0322ad0e7205a7b193368586 not-for-merge branch 'master' >of https://github.com/git/git >01d371f741e6f0b0076d9ed942d99bdb23757eac not-for-merge branch 'next' of >https://github.com/git/git >7a81cbc028be113058e0b55062706ec6de62ed94 not-for-merge branch 'pu' of >https://github.com/git/git >722746685bce03f223ed75febe312495e6c139da not-for-merge branch 'todo' of >https://github.com/git/git > This is the most interesting part of the issue. I **did not** run 'git fetch ...' in between those cat commands. I was surprised by how the contents of FETCH_HEAD are changing without me spawning any git processes that might change it. Am I missing something here? As far as i could remember, there wasn't any IDE running that might automatically spawn git commands especially in that work tree. Weird. -- Sivaraam Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: [PATCH 1/8] sha1-array: provide oid_array_filter
On Tue, Sep 25, 2018 at 12:26:44PM -0700, Stefan Beller wrote:
> On Sat, Sep 22, 2018 at 5:58 AM Ævar Arnfjörð Bjarmason
> wrote:
> >
> >
> > On Fri, Sep 21 2018, Stefan Beller wrote:
> >
> > > +/*
> > > + * Apply want to each entry in array, retaining only the entries for
> > > + * which the function returns true. Preserve the order of the entries
> > > + * that are retained.
> > > + */
> > > +void oid_array_filter(struct oid_array *array,
> > > + for_each_oid_fn want,
> > > + void *cbdata);
> > > +
> > > #endif /* SHA1_ARRAY_H */
> >
> > The code LGTM, but this comment should instead be an update to the API
> > docs, see my recent 5cc044e025 ("get_short_oid: sort ambiguous objects
> > by type, then SHA-1", 2018-05-10) for an addition of a new function to
> > this API where I added some new docs.
>
> ok will fix for consistency (this whole API is there).
>
> Longer term (I thought) we were trying to migrate API docs
> to headers instead?
Yes, please. I think it prevents exactly this sort of confusion. :)
-Peff
Re: [PATCH v4 9/9] Documentation/config: add odb..promisorRemote
On Tue, Sep 25, 2018 at 03:31:36PM -0700, Junio C Hamano wrote: > Christian Couder writes: > > > The main issue that this patch series tries to solve is that > > extensions.partialclone config option limits the partial clone and > > promisor features to only one remote. One related issue is that it > > also prevents to have other kind of promisor/partial clone/odb > > remotes. By other kind I mean remotes that would not necessarily be > > git repos, but that could store objects (that's where ODB, for Object > > DataBase, comes from) and could provide those objects to Git through a > > helper (or driver) script or program. > > I do not think "sources that are not git repositories" is all that > interesting, unless they can also serve as the source for ext:: > remote helper. And if they can serve "git fetch ext::...", I think > they can be treated just like a normal Git repository by the > backfill code that needs to lazily populate the partial clone. I don't know about that. Imagine I had a regular Git repo with a bunch of large blobs, and then I also stored those large blobs in something like S3 that provides caching, geographic locality, and resumable transfers. It would be nice to be able to say: 1. Clone from the real repo, but do not transfer any blobs larger than 10MB. 2. When you need a blob, check the external odb that points to S3. Git cannot know about this automatically, but presumably you would set a few config variables to point to an external-odb helper script. 3. If for some reason S3 doesn't work, you can always request it from the original repo. That part _doesn't_ need extra config, since we can assume that the source of the promisor pack can feed us the extra objects[1]. But you don't need to ever be able to "git fetch" from the S3 repo. Now if you are arguing that the interface to the external-odb helper script should be that it _looks_ like upload-pack, but simply advertises no refs and will let you fetch any object, that makes more sense to me. It's not something you could "git clone", but you can "git fetch" from it. However, that may be an overly constricting interface for the helper. E.g., we might want to be able to issue several requests and have them transfer in parallel. But I suppose we could teach that trick to upload-pack in the long run, as it may be applicable even to fetching from "real" git repos. Hmm. Actually, I kind of like that direction the more I think about it. -Peff
Re: git fetch behaves weirdely when run in a worktree
On 26 September 2018 03:10:17 GMT+05:30, Junio C Hamano wrote: > > That looks like fetching only the 'next' branch and nothing else to > me. > Interesting. > Perhaps your script is referring to a variable whose assignment is > misspelled and invoking > > git fetch $origin $branch > > and you are expecting the $branch variable to have string 'next' but > due to some bugs it is empty somehow? That explains why sometimes > (i.e. when $branch does not get the value you expect it to have) the > script fetches everything and some other times (i.e. when $branch > does get the right value) the script fetches only the named branch. Noce guees but I should have made this clear. The weirdness I described in my initial mail happens when I run "git fetch origin next" manually in the terminal. The script only helped me identity that there was an issue as it was automatically building the wrong version of Git. It was building and installing the current 'origin/maint' when I wrote it for building origin/next. I could easily identify the difference as 'origin/maint' was at v2.18 while 'origin/next' was at v2.19 when I notived this issue. Also the script doesn't depend on any variables for the branch name. I've hardcoded the 'next' branch into it. For the sake of reference, I've attached the script inline at the end of this mail. Note that I've sanitized the path in which the worktree exists as $COMMON_ROOT. I did not notice this weirdness in another PC. But, this happens in all the worktrees other than the main worktree in my laptop. I'm not sure what I'm doing weirdly that might have caused this issue. I'm not sure whether I mentioned this before I'm currently using a manually built version of Git. It was built from origin/next pointing at 01d371f741. But this also happens in Git v2.18.0 that comes via the pacakge manager of my operating system (Debian testing). -- Sivaraam #!/bin/sh # # A script for the cron job that automatically fetches # updates for the 'next' branch of Git and builds and # installs the binary from source. # # The build succeeds only if the config.mak is present # in the directory with appropriate configuration. # # The binary is installed into the default location if # a prefix is not specified in the config.mak file. # Bringing the binary into use is in the hands of the user. # Hint: A bash alias might help. GIT_NEXT_BUILD_AUTOMATE_DIR='$COMMON_ROOT/git-next-build-automate' LOG_FILE="$GIT_NEXT_BUILD_AUTOMATE_DIR/GIT-NEXT-INSTALLATION-LOG.txt" LOG_MSG_COMMON="installation of git 'next' build on $(date)" if cd "$GIT_NEXT_BUILD_AUTOMATE_DIR" then # Fetch the remote changes if git fetch origin next && git checkout -f FETCH_HEAD then if make install then GIT_NEXT_BUILD_STATUS=0 else GIT_NEXT_BUILD_STATUS=1 fi else GIT_NEXT_BUILD_STATUS=1 fi else GIT_NEXT_BUILD_STATUS=1 fi if test $GIT_NEXT_BUILD_STATUS -eq 0 then echo "SUCCESS: $LOG_MSG_COMMON" >>"$LOG_FILE" else echo "FAILURE: $LOG_MSG_COMMON" >>"$LOG_FILE" fi
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Tue, Sep 25, 2018 at 06:09:35PM -0700, Taylor Blau wrote:
> > So I think this is fine (modulo that the grep and sed can be combined).
> > Yet another option would be to simply strip away everything except the
> > object id (which is all we care about), like:
> >
> > depacketize | perl -lne '/^(\S+) \.have/ and print $1'
>
> Thanks for this. This is the suggestion I ended up taking (modulo taking
> '-' as the first argument to 'depacketize').
I don't think depacketize takes any arguments. It always reads from
stdin directly, doesn't it? Your "-" is not hurting anything, but it is
totally ignored.
A perl tangent if you're interested:
Normally for shell functions like this that are just wrappers around
perl snippets, I would suggest to pass "$@" from the function's
arguments to perl. So for example if we had:
haves_from_packets () {
perl -lne '/^(\S+) \.have/ and print $1' "$@"
}
then you could call it with a filename:
haves_from_packets packets
or input on stdin:
haves_from_packets )" explicitly in your program).
But because depacketize() has to use byte-wise read() calls, it
doesn't get that magic for free. And it did not seem worth the effort
to implement, when shell redirections are so easy. ;)
Just skimming through test-lib-functions.sh, though, it does seem that
we often deviate from that pattern (e.g., all of the q_to_nul family).
And has seemed to mind.
> The 'print $1' part of this makes things a lot nicer, actually, having
> removed the " .have" suffix. We can get rid of the expect_haves()
> function above, and instead call 'git rev-parse' inline and get the
> right results.
Yes. You can even do it all in a single rev-parse call.
-Peff
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Tue, Sep 25, 2018 at 06:06:06PM -0700, Taylor Blau wrote:
> > > +extract_haves () {
> > > + depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
> > > +}
> >
> > Don't pipe grep into sed, especially when both the pattern to filter
> > and the operation to perform are simple.
> >
> > I am not sure what you are trying to achive with 'g' in
> > s/pattern$//g; The anchor at the rightmost end of the pattern makes
> > sure that the pattern matches only once per line at the end anyway,
> > so "do this howmanyever times as we have match on each line" would
> > not make any difference, no?
>
> I admit to not fully understanding when the trailing `/g` is and is not
> useful. Anyway, I took Peff's suggestion below to convert this 'grep |
> sed' pipeline into a Perl invocation, which I think ended up much
> cleaner.
It makes the replacement global in the line. Without we substitute only
the first match. So try:
echo foo | sed s/o/X/
versus:
echo foo | sed s/o/X/g
-Peff
Re: [PATCH v2 3/3] transport.c: introduce core.alternateRefsPrefixes
On Tue, Sep 25, 2018 at 04:56:11PM -0700, Junio C Hamano wrote:
> Taylor Blau writes:
>
> > My reading of this is threefold:
> >
> > 1. There are some cosmetic changes that need to occur in t5410 and
> > documentation, which are mentioned above. Those seem self
> > explanatory, and I've applied the necessary bits already on my
> > local version of this topic.
> >
> > 2. The core.alternateRefsCommand vs transport.* discussion was
> > resolved in [1] as "let's use core.alternateRefsCommand and
> > core.alternateRefsPrefixes" for now, and others contributors can
> > change this as is needed.
> >
> > 3. We can apply Peff's patch to remove the refname requirement before
> > mine, as well as any relevant changes in my series as have been
> > affected by Peff's patch (e.g., documentation mentioning
> > '%(refname)', etc).
Yeah, these three sound right to me.
> I do think it makes sense to allow alternateRefsCommand to output
> just the object names without adding any refnames, and to keep the
> parser simple, we should not even make the refname optional
> (i.e. "allow" above becomes "require"), and make the default one
> done via an invocation of for-each-ref also do the same.
Yeah, making it optional is just the worst of both worlds, IMHO. Then
callers sometimes get a real value and sometimes just whatever garbage
we fill in, and can't rely on it.
> I do not think there was a strong concensus that we need to change
> the internal C API signature, though. If the function signature for
> the callback between each_ref_fn and alternate_ref_fn were the same,
> I would have opposed to the change, but because they are already
> different, I do not think it is necessary to keep the dummy refname
> parameter that is always passed a meaningless value.
Agreed. I adjusted my "rev-list --alternate-refs" patch for the proposed
new world order (just because it's the likely user of the refname
field). Since the function signatures aren't the same, I already had a
custom callback. It did chain to the existing each_ref_fn one, so I had
to adjust it like so:
diff --git a/revision.c b/revision.c
index 3988275fde..8dfe2fd4c0 100644
--- a/revision.c
+++ b/revision.c
@@ -1396,11 +1396,10 @@ void add_index_objects_to_pending(struct rev_info
*revs, unsigned int flags)
free_worktrees(worktrees);
}
-static void handle_one_alternate_ref(const char *refname,
-const struct object_id *oid,
+static void handle_one_alternate_ref(const struct object_id *oid,
void *data)
{
- handle_one_ref(refname, oid, 0, data);
+ handle_one_ref(".have", oid, 0, data);
}
static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
But I think that's fine. We have to handle the lack of name _somewhere_
in the call stack, so I'd just as soon it be here in the callback, where
we know what it will be used for (or not used at all).
> The final series would be
>
> 1/4: peff's "refnames in alternates do nto matter"
>
> 2/4: your "hardcoded for-each-ref becomes just a default"
>
> 3/4: your "config can affect what command enumerates alternate's tips"
>
> 4/4: your "with prefix config, you don't need a fully custom command"
Yep, that's what I'd expect from the new series.
-Peff
URGENT RESPONSE PLEASE.
Hello my dear. Did you receive my email message to you? Please, get back to me ASAP as the matter is becoming late. Expecting your urgent response. Sean.
Re: [PATCH v2 3/3] transport.c: introduce core.alternateRefsPrefixes
On Tue, Sep 25, 2018 at 04:56:11PM -0700, Junio C Hamano wrote: > Taylor Blau writes: > > > My reading of this is threefold: > > > > 1. There are some cosmetic changes that need to occur in t5410 and > > documentation, which are mentioned above. Those seem self > > explanatory, and I've applied the necessary bits already on my > > local version of this topic. > > > > 2. The core.alternateRefsCommand vs transport.* discussion was > > resolved in [1] as "let's use core.alternateRefsCommand and > > core.alternateRefsPrefixes" for now, and others contributors can > > change this as is needed. > > > > 3. We can apply Peff's patch to remove the refname requirement before > > mine, as well as any relevant changes in my series as have been > > affected by Peff's patch (e.g., documentation mentioning > > '%(refname)', etc). > > I do think it makes sense to allow alternateRefsCommand to output > just the object names without adding any refnames, and to keep the > parser simple, we should not even make the refname optional > (i.e. "allow" above becomes "require"), and make the default one > done via an invocation of for-each-ref also do the same. > > I do not think there was a strong concensus that we need to change > the internal C API signature, though. If the function signature for > the callback between each_ref_fn and alternate_ref_fn were the same, > I would have opposed to the change, but because they are already > different, I do not think it is necessary to keep the dummy refname > parameter that is always passed a meaningless value. > > The final series would be > > 1/4: peff's "refnames in alternates do nto matter" > > 2/4: your "hardcoded for-each-ref becomes just a default" > > 3/4: your "config can affect what command enumerates alternate's tips" > > 4/4: your "with prefix config, you don't need a fully custom command" > > I guess? Perfect -- we are in agreement on how the rerolled series should be organized. I don't anticipate much further comment on v2 in this thread, but I'll let it sit overnight to make sure that the dust has all settled after my new mail. I have a version of what will likely become 'v3', pushed here: [1]. Thanks, Taylor [1]: https://github.com/ttaylorr/git/tree/tb/alternate-refs-cmd
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Sat, Sep 22, 2018 at 03:52:58PM -0400, Jeff King wrote:
> On Sat, Sep 22, 2018 at 06:02:31PM +, brian m. carlson wrote:
>
> > On Fri, Sep 21, 2018 at 02:47:43PM -0400, Taylor Blau wrote:
> > > +expect_haves () {
> > > + printf "%s .have\n" $(git rev-parse $@) >expect
> > > +}
> > > +
> > > +extract_haves () {
> > > + depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
> >
> > It looks like you're trying to match a NUL here in the sed expression,
> > but from my reading of it, POSIX doesn't permit BREs to match NUL.
>
> No, it's trying to literally match backslash followed by 0. The
> depacketize() script will have undone the NUL already. In perl, no less,
> making it more or less equivalent to your suggestion. ;)
>
> So I think this is fine (modulo that the grep and sed can be combined).
> Yet another option would be to simply strip away everything except the
> object id (which is all we care about), like:
>
> depacketize | perl -lne '/^(\S+) \.have/ and print $1'
Thanks for this. This is the suggestion I ended up taking (modulo taking
'-' as the first argument to 'depacketize').
The 'print $1' part of this makes things a lot nicer, actually, having
removed the " .have" suffix. We can get rid of the expect_haves()
function above, and instead call 'git rev-parse' inline and get the
right results.
> Or the equivalent in sed. I am happy with any solution that does the
> correct thing.
Me too :-). Thanks again.
Thanks,
Taylor
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 02:09:08PM -0700, Junio C Hamano wrote:
> Taylor Blau writes:
>
> > +core.alternateRefsCommand::
> > + When listing references from an alternate (e.g., in the case of
> > ".have"), use
>
> It is not clear how (e.g.,...) connects to what is said in the
> sentence. "When advertising tips of available history from an
> alternate, use ..." without saying ".have" may be less cryptic.
>
> I dunno.
Thanks, I think that I tend to overuse both "e.g.," and "i.e.,". I took
your suggestion as above, which I think looks better than my original
prose.
> > + the shell to execute the specified command instead of
> > + linkgit:git-for-each-ref[1]. The first argument is the path of the
> > alternate.
>
> "The path" meaning the absolute path? Relative to the original
> object store? Something else?
It's the absolute path, and I've updated the documentation to clarify it
as such.
> > + Output must be of the form: `%(objectname) SPC %(refname)`.
> > ++
> > +This is useful when a repository only wishes to advertise some of its
> > +alternate's references as ".have"'s. For example, to only advertise branch
> > +heads, configure `core.alternateRefsCommand` to the path of a script which
> > runs
> > +`git --git-dir="$1" for-each-ref refs/heads`.
> > +
> > core.bare::
> > If true this repository is assumed to be 'bare' and has no
> > working directory associated with it. If this is the case a
> > diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> > new file mode 100755
> > index 00..2f21f1cb8f
> > --- /dev/null
> > +++ b/t/t5410-receive-pack.sh
> > @@ -0,0 +1,54 @@
> > +#!/bin/sh
> > +
> > +test_description='git receive-pack test'
> > +
> > +. ./test-lib.sh
> > +
> > +test_expect_success 'setup' '
> > + test_commit one &&
> > + git update-ref refs/heads/a HEAD &&
> > + test_commit two &&
> > + git update-ref refs/heads/b HEAD &&
> > + test_commit three &&
> > + git update-ref refs/heads/c HEAD &&
> > + git clone --bare . fork &&
> > + git clone fork pusher &&
> > + (
> > + cd fork &&
> > + git config receive.advertisealternates true &&
>
> Hmph. Do we have code to support this configuration variable?
We don't ;-). Peff's explanation of why is accurate, and the mistake is
mine.
> > + cat <<-EOF | git update-ref --stdin &&
>
> Style: writing "<<-\EOF" instead would allow readers' eyes to
> coast over without having to look for $variable_references in
> the here-doc.
>
> > + delete refs/heads/a
> > + delete refs/heads/b
> > + delete refs/heads/c
> > + delete refs/heads/master
> > + delete refs/tags/one
> > + delete refs/tags/two
> > + delete refs/tags/three
Thanks, it ended up being much cleaner to write <<-\EOF, and avoid the
unnecessary cat(1) entirely.
> So, the original created one/two/three/a/b/c/master, fork is a bare
> clone of it and has all these things, and then you deleted all of
> these? What does fork have after this is done? HEAD that is
> dangling?
>
> > + EOF
> > + echo "../../.git/objects" >objects/info/alternates
>
> When viewed from fork/objects, ../../.git is the GIT_DIR of the
> primary test repository, so that is where we borrow objects from.
>
> If we pruned the objects from fork's object store before this echo,
> we would have an almost empty repository that borrows from its
> alternates everything, which may make a more realistic sample case,
> but because you are only focusing on the ref advertisement, it does
> not matter that your fork is full of duplicate objects that are
> available from the alternates.
I could go either way. You're right in that we have only a dangling HEAD
reference in the fork, and that all of the objects are still there. I
suppose that we could gc the objects that are there, but I think (as you
note above) that it doesn't make a huge difference either way.
> > +expect_haves () {
> > + printf "%s .have\n" $(git rev-parse $@) >expect
>
> Quote $@ inside dq pair, like $(git rev-parse "$@").
Thanks, I fixed this (per your and Eric's suggestion), but ended up
removing the function entirely anyway.
> > +extract_haves () {
> > + depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
> > +}
>
> Don't pipe grep into sed, especially when both the pattern to filter
> and the operation to perform are simple.
>
> I am not sure what you are trying to achive with 'g' in
> s/pattern$//g; The anchor at the rightmost end of the pattern makes
> sure that the pattern matches only once per line at the end anyway,
> so "do this howmanyever times as we have match on each line" would
> not make any difference, no?
I admit to not fully understanding when the trailing `/g` is and is not
useful. Anyway, I took Peff's suggestion below to convert this 'grep |
sed' pipeline into a Perl invocation, which I think ended up much
cleaner.
Thanks,
Taylor
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 04:18:03PM -0400, Eric Sunshine wrote:
> On Fri, Sep 21, 2018 at 2:47 PM Taylor Blau wrote:
> > When in a repository containing one or more alternates, Git would
> > sometimes like to list references from its alternates. For example, 'git
> > receive-pack' list the objects pointed to by alternate references as
> > special ".have" references.
> > [...]
> > Signed-off-by: Taylor Blau
> > ---
> > diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> > @@ -0,0 +1,54 @@
> > +expect_haves () {
> > + printf "%s .have\n" $(git rev-parse $@) >expect
> > +}
>
> Magic quoting behavior only kicks in when $@ is itself quoted, so this
> should be:
>
> printf "%s .have\n" $(git rev-parse "$@") >expect
>
> However, as it's unlikely that you need magic quoting in this case,
> you might get by with plain $* (unquoted).
Yep, thanks for catching my mistake. I rewrote my local copy with "$@"
(instead of $@), and also applied your suggestion of not redirecting to
`>expect`, and renaming the function.
These both ended up becoming moot points, though, because of the
Perl-ism that Peff suggested and I adopted throughout this thread.
The Perl Peff wrote does not capture the " .have" suffix at all, and
instead only the object identifiers. Hence, all we really need is a call
to 'git-rev-parse(1)'. I doubt that this will ever change, so I removed
the function entirely.
Thanks,
Taylor
Re: [PATCH 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 12:59:16PM -0700, Junio C Hamano wrote:
> Taylor Blau writes:
>
> > In fact, I think that we can go even further: since we don't need to
> > catch the beginning '^.*' (without -o), we can instead:
> >
> > extract_haves () {
> > depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
> > }
>
> Do not pipe grep into sed, unless you have an overly elaborate set
> of patterns to filter with, e.g. something along the lines of...
>
> sed -ne '/\.have/s/...//p'
Thanks, I'm not sure why I thought that this was a good idea to send
(even after discussing it to myself twice publicly on the list
beforehand).
Anyway, in my local copy, I adopted Peff's suggestion below in the
thread, which is:
extract_haves () {
depacketize - | perl -lne '/^(\S+) \.have/ and print $1'
}
I think that that should be OK, but I sent it here to double check
before sending you real patches.
Thanks,
Taylor
[no subject]
-- Did you receive our proposal email ?
Re: [PATCH v2 3/3] transport.c: introduce core.alternateRefsPrefixes
Taylor Blau writes: > My reading of this is threefold: > > 1. There are some cosmetic changes that need to occur in t5410 and > documentation, which are mentioned above. Those seem self > explanatory, and I've applied the necessary bits already on my > local version of this topic. > > 2. The core.alternateRefsCommand vs transport.* discussion was > resolved in [1] as "let's use core.alternateRefsCommand and > core.alternateRefsPrefixes" for now, and others contributors can > change this as is needed. > > 3. We can apply Peff's patch to remove the refname requirement before > mine, as well as any relevant changes in my series as have been > affected by Peff's patch (e.g., documentation mentioning > '%(refname)', etc). I do think it makes sense to allow alternateRefsCommand to output just the object names without adding any refnames, and to keep the parser simple, we should not even make the refname optional (i.e. "allow" above becomes "require"), and make the default one done via an invocation of for-each-ref also do the same. I do not think there was a strong concensus that we need to change the internal C API signature, though. If the function signature for the callback between each_ref_fn and alternate_ref_fn were the same, I would have opposed to the change, but because they are already different, I do not think it is necessary to keep the dummy refname parameter that is always passed a meaningless value. The final series would be 1/4: peff's "refnames in alternates do nto matter" 2/4: your "hardcoded for-each-ref becomes just a default" 3/4: your "config can affect what command enumerates alternate's tips" 4/4: your "with prefix config, you don't need a fully custom command" I guess?
Re: [RFC PATCH] transport: list refs before fetch if necessary
Jonathan Tan writes:
> diff --git a/transport-helper.c b/transport-helper.c
> index 143ca008c8..7213fa0d32 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -1105,6 +1105,7 @@ static struct ref *get_refs_list(struct transport
> *transport, int for_push,
> }
>
> static struct transport_vtable vtable = {
> + 0,
> set_helper_option,
> get_refs_list,
> fetch,
> diff --git a/transport-internal.h b/transport-internal.h
> index 1cde6258a7..004bee5e36 100644
> --- a/transport-internal.h
> +++ b/transport-internal.h
> @@ -6,6 +6,12 @@ struct transport;
> struct argv_array;
>
> struct transport_vtable {
> + /**
> + * This transport supports the fetch() function being called
> + * without get_refs_list() first being called.
> + */
> + unsigned fetch_without_list : 1;
> +
> /**
>* Returns 0 if successful, positive if the option is not
>* recognized or is inapplicable, and negative if the option
> diff --git a/transport.c b/transport.c
> index 1c76d64aba..ee8a78ff37 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -703,6 +703,7 @@ static int disconnect_git(struct transport *transport)
> }
>
> static struct transport_vtable taken_over_vtable = {
> + 1,
> NULL,
> get_refs_via_connect,
> fetch_refs_via_pack,
> @@ -852,6 +853,7 @@ void transport_check_allowed(const char *type)
> }
>
> static struct transport_vtable bundle_vtable = {
> + 0,
> NULL,
> get_refs_from_bundle,
> fetch_refs_from_bundle,
> @@ -861,6 +863,7 @@ static struct transport_vtable bundle_vtable = {
> };
>
> static struct transport_vtable builtin_smart_vtable = {
> + 1,
> NULL,
> get_refs_via_connect,
> fetch_refs_via_pack,
Up to this point I think I understand the change. We gain one new
trait for each transport, many of the transport cannot run fetch
without first seeing the advertisement, some are OK, so we have 0 or
1 in these vtables as appropriately.
> @@ -1224,6 +1227,15 @@ int transport_fetch_refs(struct transport *transport,
> struct ref *refs)
> struct ref **heads = NULL;
> struct ref *rm;
>
> + if (!transport->vtable->fetch_without_list)
> + /*
> + * Some transports (e.g. the built-in bundle transport and the
> + * transport helper interface) do not work when fetching is
> + * done immediately after transport creation. List the remote
> + * refs anyway (if not already listed) as a workaround.
> + */
> + transport_get_remote_refs(transport, NULL);
> +
But this I do not quite understand. It looks saying "when asked to
fetch, if the transport does not allow us to do so without first
getting the advertisement, lazily do that", and that may be a good
thing to do, but then aren't the current set of callers already
calling transport-get-remote-refs elsewhere before they call
transport-fetch-refs? IOW, I would have expected to see a matching
removal, or at least a code that turns an unconditional call to
get-remote-refs to a conditional one that is done only for the
transport that lacks the capability, or something along that line.
... ah, do you mean that this is not a new feature, but is a bugfix
for some callers that are not calling get-remote-refs before calling
fetch-refs, and the bit is to work around the fact that some
transport not just can function without get-remote-refs first but do
not want to call it?
IOW, I am a bit confused by this comment (copied from an earlier part)
> + /**
> + * This transport supports the fetch() function being called
> + * without get_refs_list() first being called.
> + */
Shouldn't it read more like "this transport does not want its
get-refs-list called when fetch-refs is done"?
I dunno.
> for (rm = refs; rm; rm = rm->next) {
> nr_refs++;
> if (rm->peer_ref &&
[RFC PATCH] transport: list refs before fetch if necessary
The built-in bundle transport and the transport helper interface do not
work when transport_fetch_refs() is called immediately after transport
creation.
Evidence: fetch_refs_from_bundle() relies on data->header being
initialized in get_refs_from_bundle(), and fetch() in transport-helper.c
relies on either data->fetch or data->import being set by get_helper(),
but neither transport_helper_init() nor fetch() calls get_helper().
Up until the introduction of the partial clone feature, this has not
been a problem, because transport_fetch_refs() is always called after
transport_get_remote_refs(). With the introduction of the partial clone
feature, which involves calling transport_fetch_refs() (to fetch objects
by their OIDs) without transport_get_remote_refs(), this is still not a
problem, but only coincidentally - we do not support partially cloning a
bundle, and as for cloning using a transport-helper-using protocol, it
so happens that before transport_fetch_refs() is called, fetch_refs() in
fetch-object.c calls transport_set_option(), which means that the
aforementioned get_helper() is invoked through set_helper_option() in
transport-helper.c. In the future, though, there may be other use cases
in which we want to fetch without requiring listing of remote refs, so
this is still worth fixing.
This could be fixed by fixing the transports themselves, but it doesn't
seem like a good idea to me to open up previously untested code paths;
also, there may be transport helpers in the wild that assume that "list"
is always called before "fetch". Instead, fix this by having
transport_fetch_refs() call transport_get_remote_refs() to ensure that
the latter is always called at least once, unless the transport
explicitly states that it supports fetching without listing refs.
Signed-off-by: Jonathan Tan
---
I discovered this while investigating the possibility of taking
advantage of the fact that protocol v2 allows us to fetch without first
invoking ls-refs. This is useful both when lazily fetching to a partial
clone, and when invoking "git fetch --no-tags " (note
that tag following must be disabled).
Any comments on this (for or against) is appreciated, and suggestions of
better approaches are appreciated too.
---
transport-helper.c | 1 +
transport-internal.h | 6 ++
transport.c | 12
3 files changed, 19 insertions(+)
diff --git a/transport-helper.c b/transport-helper.c
index 143ca008c8..7213fa0d32 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1105,6 +1105,7 @@ static struct ref *get_refs_list(struct transport
*transport, int for_push,
}
static struct transport_vtable vtable = {
+ 0,
set_helper_option,
get_refs_list,
fetch,
diff --git a/transport-internal.h b/transport-internal.h
index 1cde6258a7..004bee5e36 100644
--- a/transport-internal.h
+++ b/transport-internal.h
@@ -6,6 +6,12 @@ struct transport;
struct argv_array;
struct transport_vtable {
+ /**
+* This transport supports the fetch() function being called
+* without get_refs_list() first being called.
+*/
+ unsigned fetch_without_list : 1;
+
/**
* Returns 0 if successful, positive if the option is not
* recognized or is inapplicable, and negative if the option
diff --git a/transport.c b/transport.c
index 1c76d64aba..ee8a78ff37 100644
--- a/transport.c
+++ b/transport.c
@@ -703,6 +703,7 @@ static int disconnect_git(struct transport *transport)
}
static struct transport_vtable taken_over_vtable = {
+ 1,
NULL,
get_refs_via_connect,
fetch_refs_via_pack,
@@ -852,6 +853,7 @@ void transport_check_allowed(const char *type)
}
static struct transport_vtable bundle_vtable = {
+ 0,
NULL,
get_refs_from_bundle,
fetch_refs_from_bundle,
@@ -861,6 +863,7 @@ static struct transport_vtable bundle_vtable = {
};
static struct transport_vtable builtin_smart_vtable = {
+ 1,
NULL,
get_refs_via_connect,
fetch_refs_via_pack,
@@ -1224,6 +1227,15 @@ int transport_fetch_refs(struct transport *transport,
struct ref *refs)
struct ref **heads = NULL;
struct ref *rm;
+ if (!transport->vtable->fetch_without_list)
+ /*
+* Some transports (e.g. the built-in bundle transport and the
+* transport helper interface) do not work when fetching is
+* done immediately after transport creation. List the remote
+* refs anyway (if not already listed) as a workaround.
+*/
+ transport_get_remote_refs(transport, NULL);
+
for (rm = refs; rm; rm = rm->next) {
nr_refs++;
if (rm->peer_ref &&
--
2.19.0.605.g01d371f741-goog
Re: [PATCH v2 3/3] transport.c: introduce core.alternateRefsPrefixes
On Tue, Sep 25, 2018 at 10:41:18AM -0700, Junio C Hamano wrote: > Jeff King writes: > > > Right, I think that is totally fine for the current uses. I guess my > > question was: do you envision cutting the interface down to only the > > oids to bite us in the future? > > > > I was on the fence during past discussions, but I think I've come over > > to the idea that the refnames actively confuse things. > > [ ... ] > > So, I think we probably are better off without names. Sorry for re-entering the thread a little later. I was travelling yesterday, and was surprised when I discovered that our "grep | sed" vs. "sed" discussion had grown so much ;-). My reading of this is threefold: 1. There are some cosmetic changes that need to occur in t5410 and documentation, which are mentioned above. Those seem self explanatory, and I've applied the necessary bits already on my local version of this topic. 2. The core.alternateRefsCommand vs transport.* discussion was resolved in [1] as "let's use core.alternateRefsCommand and core.alternateRefsPrefixes" for now, and others contributors can change this as is needed. 3. We can apply Peff's patch to remove the refname requirement before mine, as well as any relevant changes in my series as have been affected by Peff's patch (e.g., documentation mentioning '%(refname)', etc). Does this all sound sane to you (and match your recollection/reading of the thread)? If so, I'll send v3 hopefully tomorrow. Sorry for repeating what's already been said in this thread, but I felt it was important to ensure that we had matching understandings of one another. Thanks, Taylor [1]: https://public-inbox.org/git/xmqqa7o6skkl@gitster-ct.c.googlers.com/
[PATCH v9 07/21] stash: convert apply to builtin
From: Joel Teichroeb
Add a builtin helper for performing stash commands. Converting
all at once proved hard to review, so starting with just apply
lets conversion get started without the other commands being
finished.
The helper is being implemented as a drop in replacement for
stash so that when it is complete it can simply be renamed and
the shell script deleted.
Delete the contents of the apply_stash shell function and replace
it with a call to stash--helper apply until pop is also
converted.
Signed-off-by: Joel Teichroeb
Signed-off-by: Paul-Sebastian Ungureanu
---
.gitignore | 1 +
Makefile| 1 +
builtin.h | 1 +
builtin/stash--helper.c | 452
git-stash.sh| 78 +--
git.c | 1 +
6 files changed, 463 insertions(+), 71 deletions(-)
create mode 100644 builtin/stash--helper.c
diff --git a/.gitignore b/.gitignore
index ffceea7d59..b59661cb88 100644
--- a/.gitignore
+++ b/.gitignore
@@ -157,6 +157,7 @@
/git-show-ref
/git-stage
/git-stash
+/git-stash--helper
/git-status
/git-stripspace
/git-submodule
diff --git a/Makefile b/Makefile
index d03df31c2a..f900c68e69 100644
--- a/Makefile
+++ b/Makefile
@@ -1093,6 +1093,7 @@ BUILTIN_OBJS += builtin/shortlog.o
BUILTIN_OBJS += builtin/show-branch.o
BUILTIN_OBJS += builtin/show-index.o
BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stash--helper.o
BUILTIN_OBJS += builtin/stripspace.o
BUILTIN_OBJS += builtin/submodule--helper.o
BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 99206df4bd..317bc338f7 100644
--- a/builtin.h
+++ b/builtin.h
@@ -223,6 +223,7 @@ extern int cmd_show(int argc, const char **argv, const char
*prefix);
extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
extern int cmd_show_index(int argc, const char **argv, const char *prefix);
extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
extern int cmd_submodule__helper(int argc, const char **argv, const char
*prefix);
extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
new file mode 100644
index 00..7819dae332
--- /dev/null
+++ b/builtin/stash--helper.c
@@ -0,0 +1,452 @@
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+#include "refs.h"
+#include "lockfile.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "merge-recursive.h"
+#include "argv-array.h"
+#include "run-command.h"
+#include "dir.h"
+#include "rerere.h"
+
+static const char * const git_stash_helper_usage[] = {
+ N_("git stash--helper apply [--index] [-q|--quiet] []"),
+ NULL
+};
+
+static const char * const git_stash_helper_apply_usage[] = {
+ N_("git stash--helper apply [--index] [-q|--quiet] []"),
+ NULL
+};
+
+static const char *ref_stash = "refs/stash";
+static struct strbuf stash_index_path = STRBUF_INIT;
+
+/*
+ * w_commit is set to the commit containing the working tree
+ * b_commit is set to the base commit
+ * i_commit is set to the commit containing the index tree
+ * u_commit is set to the commit containing the untracked files tree
+ * w_tree is set to the working tree
+ * b_tree is set to the base tree
+ * i_tree is set to the index tree
+ * u_tree is set to the untracked files tree
+ */
+
+struct stash_info {
+ struct object_id w_commit;
+ struct object_id b_commit;
+ struct object_id i_commit;
+ struct object_id u_commit;
+ struct object_id w_tree;
+ struct object_id b_tree;
+ struct object_id i_tree;
+ struct object_id u_tree;
+ struct strbuf revision;
+ int is_stash_ref;
+ int has_u;
+};
+
+static void free_stash_info(struct stash_info *info)
+{
+ strbuf_release(>revision);
+}
+
+static void assert_stash_like(struct stash_info *info, const char *revision)
+{
+ if (get_oidf(>b_commit, "%s^1", revision) ||
+ get_oidf(>w_tree, "%s:", revision) ||
+ get_oidf(>b_tree, "%s^1:", revision) ||
+ get_oidf(>i_tree, "%s^2:", revision)) {
+ free_stash_info(info);
+ error(_("'%s' is not a stash-like commit"), revision);
+ exit(128);
+ }
+}
+
+static int get_stash_info(struct stash_info *info, int argc, const char **argv)
+{
+ int ret;
+ char *end_of_rev;
+ char *expanded_ref;
+ const char *revision;
+ const char *commit = NULL;
+ struct object_id dummy;
+ struct strbuf symbolic = STRBUF_INIT;
+
+ if (argc > 1) {
+ int i;
+ struct strbuf refs_msg = STRBUF_INIT;
+ for (i = 0; i < argc; i++)
+ strbuf_addf(_msg, " '%s'", argv[i]);
+
[GSoC][PATCH v9 06/21] strbuf.c: add `strbuf_join_argv()`
Implement `strbuf_join_argv()` to join arguments.
Signed-off-by: Paul-Sebastian Ungureanu
---
strbuf.c | 15 +++
strbuf.h | 7 +++
2 files changed, 22 insertions(+)
diff --git a/strbuf.c b/strbuf.c
index 64041c3c24..3eb431b2b0 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -259,6 +259,21 @@ void strbuf_addbuf(struct strbuf *sb, const struct strbuf
*sb2)
strbuf_setlen(sb, sb->len + sb2->len);
}
+const char *strbuf_join_argv(struct strbuf *buf,
+int argc, const char **argv, char delim)
+{
+ if (!argc)
+ return buf->buf;
+
+ strbuf_addstr(buf, *argv);
+ while (--argc) {
+ strbuf_addch(buf, delim);
+ strbuf_addstr(buf, *(++argv));
+ }
+
+ return buf->buf;
+}
+
void strbuf_addchars(struct strbuf *sb, int c, size_t n)
{
strbuf_grow(sb, n);
diff --git a/strbuf.h b/strbuf.h
index 60a35aef16..7ed859bb8a 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -284,6 +284,13 @@ static inline void strbuf_addstr(struct strbuf *sb, const
char *s)
*/
extern void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2);
+
+/**
+ *
+ */
+extern const char *strbuf_join_argv(struct strbuf *buf, int argc,
+ const char **argv, char delim);
+
/**
* This function can be used to expand a format string containing
* placeholders. To that end, it parses the string and calls the specified
--
2.19.0.rc0.23.g1c0a08a5d3
[PATCH v9 19/21] stash: convert `stash--helper.c` into `stash.c`
The old shell script `git-stash.sh` was removed and replaced
entirely by `builtin/stash.c`. In order to do that, `create` and
`push` were adapted to work without `stash.sh`. For example, before
this commit, `git stash create` called `git stash--helper create
--message "$*"`. If it called `git stash--helper create "$@"`, then
some of these changes wouldn't have been necessary.
This commit also removes the word `helper` since now stash is
called directly and not by a shell script.
Signed-off-by: Paul-Sebastian Ungureanu
---
.gitignore | 1 -
Makefile | 3 +-
builtin.h| 2 +-
builtin/{stash--helper.c => stash.c} | 162 ---
git-stash.sh | 153 -
git.c| 2 +-
6 files changed, 98 insertions(+), 225 deletions(-)
rename builtin/{stash--helper.c => stash.c} (90%)
delete mode 100755 git-stash.sh
diff --git a/.gitignore b/.gitignore
index b59661cb88..ffceea7d59 100644
--- a/.gitignore
+++ b/.gitignore
@@ -157,7 +157,6 @@
/git-show-ref
/git-stage
/git-stash
-/git-stash--helper
/git-status
/git-stripspace
/git-submodule
diff --git a/Makefile b/Makefile
index f900c68e69..ac1787d113 100644
--- a/Makefile
+++ b/Makefile
@@ -617,7 +617,6 @@ SCRIPT_SH += git-quiltimport.sh
SCRIPT_SH += git-rebase.sh
SCRIPT_SH += git-remote-testgit.sh
SCRIPT_SH += git-request-pull.sh
-SCRIPT_SH += git-stash.sh
SCRIPT_SH += git-submodule.sh
SCRIPT_SH += git-web--browse.sh
@@ -1093,7 +1092,7 @@ BUILTIN_OBJS += builtin/shortlog.o
BUILTIN_OBJS += builtin/show-branch.o
BUILTIN_OBJS += builtin/show-index.o
BUILTIN_OBJS += builtin/show-ref.o
-BUILTIN_OBJS += builtin/stash--helper.o
+BUILTIN_OBJS += builtin/stash.o
BUILTIN_OBJS += builtin/stripspace.o
BUILTIN_OBJS += builtin/submodule--helper.o
BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 317bc338f7..e60f0fc58f 100644
--- a/builtin.h
+++ b/builtin.h
@@ -223,7 +223,7 @@ extern int cmd_show(int argc, const char **argv, const char
*prefix);
extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
extern int cmd_show_index(int argc, const char **argv, const char *prefix);
extern int cmd_status(int argc, const char **argv, const char *prefix);
-extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
+extern int cmd_stash(int argc, const char **argv, const char *prefix);
extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
extern int cmd_submodule__helper(int argc, const char **argv, const char
*prefix);
extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash.c
similarity index 90%
rename from builtin/stash--helper.c
rename to builtin/stash.c
index 96689a00e9..fc4a2050b1 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash.c
@@ -16,75 +16,70 @@
#define INCLUDE_ALL_FILES 2
-static const char * const git_stash_helper_usage[] = {
- N_("git stash--helper list []"),
- N_("git stash--helper show [] []"),
- N_("git stash--helper drop [-q|--quiet] []"),
- N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet]
[]"),
- N_("git stash--helper branch []"),
- N_("git stash--helper clear"),
- N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
+static const char * const git_stash_usage[] = {
+ N_("git stash list []"),
+ N_("git stash show [] []"),
+ N_("git stash drop [-q|--quiet] []"),
+ N_("git stash ( pop | apply ) [--index] [-q|--quiet] []"),
+ N_("git stash branch []"),
+ N_("git stash clear"),
+ N_("git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
" [-u|--include-untracked] [-a|--all] [-m|--message
]\n"
" [--] [...]]"),
- N_("git stash--helper save [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
+ N_("git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
" [-u|--include-untracked] [-a|--all] []"),
NULL
};
-static const char * const git_stash_helper_list_usage[] = {
- N_("git stash--helper list []"),
+static const char * const git_stash_list_usage[] = {
+ N_("git stash list []"),
NULL
};
-static const char * const git_stash_helper_show_usage[] = {
- N_("git stash--helper show [] []"),
+static const char * const git_stash_show_usage[] = {
+ N_("git stash show [] []"),
NULL
};
-static const char * const git_stash_helper_drop_usage[] = {
- N_("git stash--helper drop [-q|--quiet] []"),
+static const char * const git_stash_drop_usage[] = {
+ N_("git stash drop [-q|--quiet] []"),
NULL
};
-static const char * const git_stash_helper_pop_usage[] = {
- N_("git stash--helper pop [--index] [-q|--quiet] []"),
[PATCH v9 14/21] stash: convert store to builtin
Add stash store to the helper and delete the store_stash function
from the shell script.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 60 +
git-stash.sh| 43 ++---
2 files changed, 62 insertions(+), 41 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 1f02f5f2e9..b7421b68aa 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -58,6 +58,11 @@ static const char * const git_stash_helper_clear_usage[] = {
NULL
};
+static const char * const git_stash_helper_store_usage[] = {
+ N_("git stash--helper store [-m|--message ] [-q|--quiet]
"),
+ NULL
+};
+
static const char *ref_stash = "refs/stash";
static struct strbuf stash_index_path = STRBUF_INIT;
@@ -728,6 +733,59 @@ static int show_stash(int argc, const char **argv, const
char *prefix)
return diff_result_code(, 0);
}
+static int do_store_stash(const struct object_id *w_commit, const char
*stash_msg,
+ int quiet)
+{
+ if (!stash_msg)
+ stash_msg = "Created via \"git stash store\".";
+
+ if (update_ref(stash_msg, ref_stash, w_commit, NULL,
+ REF_FORCE_CREATE_REFLOG,
+ quiet ? UPDATE_REFS_QUIET_ON_ERR :
+ UPDATE_REFS_MSG_ON_ERR)) {
+ if (!quiet) {
+ fprintf_ln(stderr, _("Cannot update %s with %s"),
+ ref_stash, oid_to_hex(w_commit));
+ }
+ return -1;
+ }
+
+ return 0;
+}
+
+static int store_stash(int argc, const char **argv, const char *prefix)
+{
+ int quiet = 0;
+ const char *stash_msg = NULL;
+ struct object_id obj;
+ struct object_context dummy;
+ struct option options[] = {
+ OPT__QUIET(, N_("be quiet")),
+ OPT_STRING('m', "message", _msg, "message", N_("stash
message")),
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_store_usage,
+PARSE_OPT_KEEP_UNKNOWN);
+
+ if (argc != 1) {
+ if (!quiet)
+ fprintf_ln(stderr, _("\"git stash store\" requires one
argument"));
+ return -1;
+ }
+
+ if (get_oid_with_context(argv[0], quiet ? GET_OID_QUIETLY : 0, ,
+)) {
+ if (!quiet)
+ fprintf_ln(stderr, _("Cannot update %s with %s"),
+ref_stash, argv[0]);
+ return -1;
+ }
+
+ return do_store_stash(, stash_msg, quiet);
+}
+
int cmd_stash__helper(int argc, const char **argv, const char *prefix)
{
pid_t pid = getpid();
@@ -762,6 +820,8 @@ int cmd_stash__helper(int argc, const char **argv, const
char *prefix)
return !!list_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "show"))
return !!show_stash(argc, argv, prefix);
+ else if (!strcmp(argv[0], "store"))
+ return !!store_stash(argc, argv, prefix);
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 0d05cbc1e5..5739c51527 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -191,45 +191,6 @@ create_stash () {
die "$(gettext "Cannot record working tree state")"
}
-store_stash () {
- while test $# != 0
- do
- case "$1" in
- -m|--message)
- shift
- stash_msg="$1"
- ;;
- -m*)
- stash_msg=${1#-m}
- ;;
- --message=*)
- stash_msg=${1#--message=}
- ;;
- -q|--quiet)
- quiet=t
- ;;
- *)
- break
- ;;
- esac
- shift
- done
- test $# = 1 ||
- die "$(eval_gettext "\"$dashless store\" requires one
argument")"
-
- w_commit="$1"
- if test -z "$stash_msg"
- then
- stash_msg="Created via \"git stash store\"."
- fi
-
- git update-ref --create-reflog -m "$stash_msg" $ref_stash $w_commit
- ret=$?
- test $ret != 0 && test -z "$quiet" &&
- die "$(eval_gettext "Cannot update \$ref_stash with \$w_commit")"
- return $ret
-}
-
push_stash () {
keep_index=
patch_mode=
@@ -308,7 +269,7 @@ push_stash () {
clear_stash || die "$(gettext "Cannot initialize stash")"
create_stash -m "$stash_msg" -u "$untracked" -- "$@"
- store_stash -m "$stash_msg" -q $w_commit ||
+ git
[PATCH v9 15/21] stash: convert create to builtin
Add stash create to the helper.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 450
git-stash.sh| 2 +-
2 files changed, 451 insertions(+), 1 deletion(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index b7421b68aa..49b05f2458 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,6 +12,9 @@
#include "rerere.h"
#include "revision.h"
#include "log-tree.h"
+#include "diffcore.h"
+
+#define INCLUDE_ALL_FILES 2
static const char * const git_stash_helper_usage[] = {
N_("git stash--helper list []"),
@@ -63,6 +66,11 @@ static const char * const git_stash_helper_store_usage[] = {
NULL
};
+static const char * const git_stash_helper_create_usage[] = {
+ N_("git stash--helper create []"),
+ NULL
+};
+
static const char *ref_stash = "refs/stash";
static struct strbuf stash_index_path = STRBUF_INIT;
@@ -289,6 +297,24 @@ static int reset_head(void)
return run_command();
}
+static void add_diff_to_buf(struct diff_queue_struct *q,
+ struct diff_options *options,
+ void *data)
+{
+ int i;
+
+ for (i = 0; i < q->nr; i++) {
+ strbuf_addstr(data, q->queue[i]->one->path);
+
+ /*
+* The reason we add "0" at the end of this strbuf
+* is because we will pass the output further to
+* "git update-index -z ...".
+*/
+ strbuf_addch(data, 0);
+ }
+}
+
static int get_newly_staged(struct strbuf *out, struct object_id *c_tree)
{
struct child_process cp = CHILD_PROCESS_INIT;
@@ -786,6 +812,428 @@ static int store_stash(int argc, const char **argv, const
char *prefix)
return do_store_stash(, stash_msg, quiet);
}
+static void add_pathspecs(struct argv_array *args,
+ struct pathspec ps) {
+ int i;
+
+ for (i = 0; i < ps.nr; i++)
+ argv_array_push(args, ps.items[i].match);
+}
+
+/*
+ * `untracked_files` will be filled with the names of untracked files.
+ * The return value is:
+ *
+ * = 0 if there are not any untracked files
+ * > 0 if there are untracked files
+ */
+static int get_untracked_files(struct pathspec ps, int include_untracked,
+ struct strbuf *untracked_files)
+{
+ int i;
+ int max_len;
+ int found = 0;
+ char *seen;
+ struct dir_struct dir;
+
+ memset(, 0, sizeof(dir));
+ if (include_untracked != INCLUDE_ALL_FILES)
+ setup_standard_excludes();
+
+ seen = xcalloc(ps.nr, 1);
+
+ max_len = fill_directory(, the_repository->index, );
+ for (i = 0; i < dir.nr; i++) {
+ struct dir_entry *ent = dir.entries[i];
+ if (dir_path_match(_index, ent, , max_len, seen)) {
+ found++;
+ strbuf_addstr(untracked_files, ent->name);
+ /* NUL-terminate: will be fed to update-index -z */
+ strbuf_addch(untracked_files, 0);
+ }
+ free(ent);
+ }
+
+ free(seen);
+ free(dir.entries);
+ free(dir.ignored);
+ clear_directory();
+ return found;
+}
+
+/*
+ * The return value of `check_changes()` can be:
+ *
+ * < 0 if there was an error
+ * = 0 if there are no changes.
+ * > 0 if there are changes.
+ */
+static int check_changes(struct pathspec ps, int include_untracked)
+{
+ int result;
+ int ret = 0;
+ struct rev_info rev;
+ struct object_id dummy;
+ struct strbuf out = STRBUF_INIT;
+
+ /* No initial commit. */
+ if (get_oid("HEAD", ))
+ return -1;
+
+ if (read_cache() < 0)
+ return -1;
+
+ init_revisions(, NULL);
+ rev.prune_data = ps;
+
+ rev.diffopt.flags.quick = 1;
+ rev.diffopt.flags.ignore_submodules = 1;
+ rev.abbrev = 0;
+
+ add_head_to_pending();
+ diff_setup_done();
+
+ result = run_diff_index(, 1);
+ if (diff_result_code(, result))
+ return 1;
+
+ object_array_clear();
+ result = run_diff_files(, 0);
+ if (diff_result_code(, result))
+ return 1;
+
+ if (include_untracked && get_untracked_files(ps, include_untracked,
+)) {
+ strbuf_release();
+ return 1;
+ }
+
+ strbuf_release();
+ return 0;
+}
+
+static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
+ struct strbuf files)
+{
+ int ret = 0;
+ struct strbuf untracked_msg = STRBUF_INIT;
+ struct strbuf out = STRBUF_INIT;
+ struct child_process cp_upd_index = CHILD_PROCESS_INIT;
+ struct child_process cp_write_tree = CHILD_PROCESS_INIT;
+
+
[PATCH v9 08/21] stash: convert drop and clear to builtin
From: Joel Teichroeb
Add the drop and clear commands to the builtin helper. These two
are each simple, but are being added together as they are quite
related.
We have to unfortunately keep the drop and clear functions in the
shell script as functions are called with parameters internally
that are not valid when the commands are called externally. Once
pop is converted they can both be removed.
Signed-off-by: Joel Teichroeb
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 116
git-stash.sh| 4 +-
2 files changed, 118 insertions(+), 2 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 7819dae332..72472eaeb7 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,7 +12,14 @@
#include "rerere.h"
static const char * const git_stash_helper_usage[] = {
+ N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+ N_("git stash--helper clear"),
+ NULL
+};
+
+static const char * const git_stash_helper_drop_usage[] = {
+ N_("git stash--helper drop [-q|--quiet] []"),
NULL
};
@@ -21,6 +28,11 @@ static const char * const git_stash_helper_apply_usage[] = {
NULL
};
+static const char * const git_stash_helper_clear_usage[] = {
+ N_("git stash--helper clear"),
+ NULL
+};
+
static const char *ref_stash = "refs/stash";
static struct strbuf stash_index_path = STRBUF_INIT;
@@ -139,6 +151,31 @@ static int get_stash_info(struct stash_info *info, int
argc, const char **argv)
return !(ret == 0 || ret == 1);
}
+static int do_clear_stash(void)
+{
+ struct object_id obj;
+ if (get_oid(ref_stash, ))
+ return 0;
+
+ return delete_ref(NULL, ref_stash, , 0);
+}
+
+static int clear_stash(int argc, const char **argv, const char *prefix)
+{
+ struct option options[] = {
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_clear_usage,
+PARSE_OPT_STOP_AT_NON_OPTION);
+
+ if (argc)
+ return error(_("git stash clear with parameters is
unimplemented"));
+
+ return do_clear_stash();
+}
+
static int reset_tree(struct object_id *i_tree, int update, int reset)
{
int nr_trees = 1;
@@ -424,6 +461,81 @@ static int apply_stash(int argc, const char **argv, const
char *prefix)
return ret;
}
+static int do_drop_stash(const char *prefix, struct stash_info *info, int
quiet)
+{
+ int ret;
+ struct child_process cp_reflog = CHILD_PROCESS_INIT;
+ struct child_process cp = CHILD_PROCESS_INIT;
+
+ /*
+* reflog does not provide a simple function for deleting refs. One will
+* need to be added to avoid implementing too much reflog code here
+*/
+
+ cp_reflog.git_cmd = 1;
+ argv_array_pushl(_reflog.args, "reflog", "delete", "--updateref",
+"--rewrite", NULL);
+ argv_array_push(_reflog.args, info->revision.buf);
+ ret = run_command(_reflog);
+ if (!ret) {
+ if (!quiet)
+ printf_ln(_("Dropped %s (%s)"), info->revision.buf,
+ oid_to_hex(>w_commit));
+ } else {
+ return error(_("%s: Could not drop stash entry"),
+info->revision.buf);
+ }
+
+ /*
+* This could easily be replaced by get_oid, but currently it will throw
+* a fatal error when a reflog is empty, which we can not recover from.
+*/
+ cp.git_cmd = 1;
+ /* Even though --quiet is specified, rev-parse still outputs the hash */
+ cp.no_stdout = 1;
+ argv_array_pushl(, "rev-parse", "--verify", "--quiet", NULL);
+ argv_array_pushf(, "%s@{0}", ref_stash);
+ ret = run_command();
+
+ /* do_clear_stash if we just dropped the last stash entry */
+ if (ret)
+ do_clear_stash();
+
+ return 0;
+}
+
+static void assert_stash_ref(struct stash_info *info)
+{
+ if (!info->is_stash_ref) {
+ free_stash_info(info);
+ error(_("'%s' is not a stash reference"), info->revision.buf);
+ exit(128);
+ }
+}
+
+static int drop_stash(int argc, const char **argv, const char *prefix)
+{
+ int ret;
+ int quiet = 0;
+ struct stash_info info;
+ struct option options[] = {
+ OPT__QUIET(, N_("be quiet, only report errors")),
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_drop_usage, 0);
+
+ if (get_stash_info(, argc, argv))
+ return -1;
+
+ assert_stash_ref();
+
+ ret = do_drop_stash(prefix, , quiet);
+ free_stash_info();
+ return ret;
+}
+
int
[PATCH v9 06/21] stash: add tests for `git stash show` config
This commit introduces tests for `git stash show`
config. It tests all the cases where `stash.showStat`
and `stash.showPatch` are unset or set to true / false.
Signed-off-by: Paul-Sebastian Ungureanu
---
t/t3907-stash-show-config.sh | 83
1 file changed, 83 insertions(+)
create mode 100755 t/t3907-stash-show-config.sh
diff --git a/t/t3907-stash-show-config.sh b/t/t3907-stash-show-config.sh
new file mode 100755
index 00..10914bba7b
--- /dev/null
+++ b/t/t3907-stash-show-config.sh
@@ -0,0 +1,83 @@
+#!/bin/sh
+
+test_description='Test git stash show configuration.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ test_commit file
+'
+
+# takes three parameters:
+# 1. the stash.showStat value (or "")
+# 2. the stash.showPatch value (or "")
+# 3. the diff options of the expected output (or nothing for no output)
+test_stat_and_patch () {
+ if test "" = "$1"
+ then
+ test_unconfig stash.showStat
+ else
+ test_config stash.showStat "$1"
+ fi &&
+
+ if test "" = "$2"
+ then
+ test_unconfig stash.showPatch
+ else
+ test_config stash.showPatch "$2"
+ fi &&
+
+ shift 2 &&
+ echo 2 >file.t &&
+ if test $# != 0
+ then
+ git diff "$@" >expect
+ fi &&
+ git stash &&
+ git stash show >actual &&
+
+ if test $# = 0
+ then
+ test_must_be_empty actual
+ else
+ test_cmp expect actual
+ fi
+}
+
+test_expect_success 'showStat unset showPatch unset' '
+ test_stat_and_patch "" "" --stat
+'
+
+test_expect_success 'showStat unset showPatch false' '
+ test_stat_and_patch "" false --stat
+'
+
+test_expect_success 'showStat unset showPatch true' '
+ test_stat_and_patch "" true --stat -p
+'
+
+test_expect_success 'showStat false showPatch unset' '
+ test_stat_and_patch false ""
+'
+
+test_expect_success 'showStat false showPatch false' '
+ test_stat_and_patch false false
+'
+
+test_expect_success 'showStat false showPatch true' '
+ test_stat_and_patch false true -p
+'
+
+test_expect_success 'showStat true showPatch unset' '
+ test_stat_and_patch true "" --stat
+'
+
+test_expect_success 'showStat true showPatch false' '
+ test_stat_and_patch true false --stat
+'
+
+test_expect_success 'showStat true showPatch true' '
+ test_stat_and_patch true true --stat -p
+'
+
+test_done
--
2.19.0.rc0.23.g1fb9f40d88
[PATCH v9 10/21] stash: convert pop to builtin
From: Joel Teichroeb
Add stash pop to the helper and delete the pop_stash, drop_stash,
assert_stash_ref functions from the shell script now that they
are no longer needed.
Signed-off-by: Joel Teichroeb
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 38 -
git-stash.sh| 47 ++---
2 files changed, 39 insertions(+), 46 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 5841bd0e98..c1f78d3275 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -13,7 +13,7 @@
static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
- N_("git stash--helper apply [--index] [-q|--quiet] []"),
+ N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet]
[]"),
N_("git stash--helper branch []"),
N_("git stash--helper clear"),
NULL
@@ -24,6 +24,11 @@ static const char * const git_stash_helper_drop_usage[] = {
NULL
};
+static const char * const git_stash_helper_pop_usage[] = {
+ N_("git stash--helper pop [--index] [-q|--quiet] []"),
+ NULL
+};
+
static const char * const git_stash_helper_apply_usage[] = {
N_("git stash--helper apply [--index] [-q|--quiet] []"),
NULL
@@ -542,6 +547,35 @@ static int drop_stash(int argc, const char **argv, const
char *prefix)
return ret;
}
+static int pop_stash(int argc, const char **argv, const char *prefix)
+{
+ int ret;
+ int index = 0;
+ int quiet = 0;
+ struct stash_info info;
+ struct option options[] = {
+ OPT__QUIET(, N_("be quiet, only report errors")),
+ OPT_BOOL(0, "index", ,
+N_("attempt to recreate the index")),
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_pop_usage, 0);
+
+ if (get_stash_info(, argc, argv))
+ return -1;
+
+ assert_stash_ref();
+ if ((ret = do_apply_stash(prefix, , index, quiet)))
+ printf_ln(_("The stash entry is kept in case you need it
again."));
+ else
+ ret = do_drop_stash(prefix, , quiet);
+
+ free_stash_info();
+ return ret;
+}
+
static int branch_stash(int argc, const char **argv, const char *prefix)
{
int ret;
@@ -606,6 +640,8 @@ int cmd_stash__helper(int argc, const char **argv, const
char *prefix)
return !!clear_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "drop"))
return !!drop_stash(argc, argv, prefix);
+ else if (!strcmp(argv[0], "pop"))
+ return !!pop_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "branch"))
return !!branch_stash(argc, argv, prefix);
diff --git a/git-stash.sh b/git-stash.sh
index 29d9f44255..8f2640fe90 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -554,50 +554,6 @@ assert_stash_like() {
}
}
-is_stash_ref() {
- is_stash_like "$@" && test -n "$IS_STASH_REF"
-}
-
-assert_stash_ref() {
- is_stash_ref "$@" || {
- args="$*"
- die "$(eval_gettext "'\$args' is not a stash reference")"
- }
-}
-
-apply_stash () {
- cd "$START_DIR"
- git stash--helper apply "$@"
- res=$?
- cd_to_toplevel
- return $res
-}
-
-pop_stash() {
- assert_stash_ref "$@"
-
- if apply_stash "$@"
- then
- drop_stash "$@"
- else
- status=$?
- say "$(gettext "The stash entry is kept in case you need it
again.")"
- exit $status
- fi
-}
-
-drop_stash () {
- assert_stash_ref "$@"
-
- git reflog delete --updateref --rewrite "${REV}" &&
- say "$(eval_gettext "Dropped \${REV} (\$s)")" ||
- die "$(eval_gettext "\${REV}: Could not drop stash entry")"
-
- # clear_stash if we just dropped the last stash entry
- git rev-parse --verify --quiet "$ref_stash@{0}" >/dev/null ||
- clear_stash
-}
-
test "$1" = "-p" && set "push" "$@"
PARSE_CACHE='--not-parsed'
@@ -655,7 +611,8 @@ drop)
;;
pop)
shift
- pop_stash "$@"
+ cd "$START_DIR"
+ git stash--helper pop "$@"
;;
branch)
shift
--
2.19.0.rc0.23.g1fb9f40d88
[PATCH v9 18/21] stash: convert save to builtin
Add stash save to the helper and delete functions which are no
longer needed (`show_help()`, `save_stash()`, `push_stash()`,
`create_stash()`, `clear_stash()`, `untracked_files()` and
`no_changes()`).
The `-m` option is no longer supported as it might not make
sense to have two ways of passing a message. Even if this is
a change in behaviour, the documentation remains the same
because the `-m` parameter was omitted before.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 50 +++
git-stash.sh| 311 +---
2 files changed, 52 insertions(+), 309 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 73bb22dc94..96689a00e9 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -26,6 +26,8 @@ static const char * const git_stash_helper_usage[] = {
N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
" [-u|--include-untracked] [-a|--all] [-m|--message
]\n"
" [--] [...]]"),
+ N_("git stash--helper save [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
+ " [-u|--include-untracked] [-a|--all] []"),
NULL
};
@@ -81,6 +83,12 @@ static const char * const git_stash_helper_push_usage[] = {
NULL
};
+static const char * const git_stash_helper_save_usage[] = {
+ N_("git stash--helper save [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
+ " [-u|--include-untracked] [-a|--all] []"),
+ NULL
+};
+
static const char *ref_stash = "refs/stash";
static struct strbuf stash_index_path = STRBUF_INIT;
@@ -1481,6 +1489,46 @@ static int push_stash(int argc, const char **argv, const
char *prefix)
include_untracked);
}
+static int save_stash(int argc, const char **argv, const char *prefix)
+{
+ int keep_index = -1;
+ int patch_mode = 0;
+ int include_untracked = 0;
+ int quiet = 0;
+ int ret = 0;
+ char *stash_msg = NULL;
+ struct pathspec ps;
+ struct strbuf buf = STRBUF_INIT;
+ struct option options[] = {
+ OPT_BOOL('k', "keep-index", _index,
+N_("keep index")),
+ OPT_BOOL('p', "patch", _mode,
+N_("stash in patch mode")),
+ OPT__QUIET(, N_("quiet mode")),
+ OPT_BOOL('u', "include-untracked", _untracked,
+N_("include untracked files in stash")),
+ OPT_SET_INT('a', "all", _untracked,
+ N_("include ignore files"), 2),
+ OPT_STRING('m', "message", _msg, "message",
+ N_("stash message")),
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_save_usage,
+PARSE_OPT_KEEP_DASHDASH);
+
+ if (argc)
+ stash_msg = (char*) strbuf_join_argv(, argc, argv, ' ');
+
+ memset(, 0, sizeof(ps));
+ ret = do_push_stash(ps, stash_msg, quiet, keep_index, patch_mode,
+ include_untracked);
+
+ strbuf_release();
+ return ret;
+}
+
int cmd_stash__helper(int argc, const char **argv, const char *prefix)
{
pid_t pid = getpid();
@@ -1521,6 +1569,8 @@ int cmd_stash__helper(int argc, const char **argv, const
char *prefix)
return !!create_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "push"))
return !!push_stash(argc, argv, prefix);
+ else if (!strcmp(argv[0], "save"))
+ return !!save_stash(argc, argv, prefix);
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index c3146f62ab..695f1feba3 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -36,314 +36,6 @@ else
reset_color=
fi
-no_changes () {
- git diff-index --quiet --cached HEAD --ignore-submodules -- "$@" &&
- git diff-files --quiet --ignore-submodules -- "$@" &&
- (test -z "$untracked" || test -z "$(untracked_files "$@")")
-}
-
-untracked_files () {
- if test "$1" = "-z"
- then
- shift
- z=-z
- else
- z=
- fi
- excl_opt=--exclude-standard
- test "$untracked" = "all" && excl_opt=
- git ls-files -o $z $excl_opt -- "$@"
-}
-
-clear_stash () {
- if test $# != 0
- then
- die "$(gettext "git stash clear with parameters is
unimplemented")"
- fi
- if current=$(git rev-parse --verify --quiet $ref_stash)
- then
- git update-ref -d $ref_stash $current
- fi
-}
-
-create_stash () {
- stash_msg=
- untracked=
- while test $# != 0
- do
- case "$1" in
- -m|--message)
-
[PATCH v9 01/21] sha1-name.c: add `get_oidf()` which acts like `get_oid()`
Compared to `get_oid()`, `get_oidf()` has as parameters
a pointer to `object_id`, a printf format string and
additional arguments. This will help simplify the code
in subsequent commits.
Original-idea-by: Johannes Schindelin
Signed-off-by: Paul-Sebastian Ungureanu
---
cache.h | 1 +
sha1-name.c | 19 +++
2 files changed, 20 insertions(+)
diff --git a/cache.h b/cache.h
index b1fd3d58ab..d93b2e25a5 100644
--- a/cache.h
+++ b/cache.h
@@ -1309,6 +1309,7 @@ struct object_context {
GET_OID_BLOB)
extern int get_oid(const char *str, struct object_id *oid);
+extern int get_oidf(struct object_id *oid, const char *fmt, ...);
extern int get_oid_commit(const char *str, struct object_id *oid);
extern int get_oid_committish(const char *str, struct object_id *oid);
extern int get_oid_tree(const char *str, struct object_id *oid);
diff --git a/sha1-name.c b/sha1-name.c
index c9cc1318b7..261b960bbd 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -1471,6 +1471,25 @@ int get_oid(const char *name, struct object_id *oid)
return get_oid_with_context(name, 0, oid, );
}
+/*
+ * This returns a non-zero value if the string (built using printf
+ * format and the given arguments) is not a valid object.
+ */
+int get_oidf(struct object_id *oid, const char *fmt, ...)
+{
+ va_list ap;
+ int ret;
+ struct strbuf sb = STRBUF_INIT;
+
+ va_start(ap, fmt);
+ strbuf_vaddf(, fmt, ap);
+ va_end(ap);
+
+ ret = get_oid(sb.buf, oid);
+ strbuf_release();
+
+ return ret;
+}
/*
* Many callers know that the user meant to name a commit-ish by
--
2.19.0.rc0.23.g1fb9f40d88
[PATCH v9 21/21] stash: replace all `write-tree` child processes with API calls
This commit replaces spawning `git write-tree` with API calls.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash.c | 41 -
1 file changed, 12 insertions(+), 29 deletions(-)
diff --git a/builtin/stash.c b/builtin/stash.c
index 43d0de1f13..86a7e0a5a5 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -941,9 +941,8 @@ static int save_untracked_files(struct stash_info *info,
struct strbuf *msg,
{
int ret = 0;
struct strbuf untracked_msg = STRBUF_INIT;
- struct strbuf out = STRBUF_INIT;
struct child_process cp_upd_index = CHILD_PROCESS_INIT;
- struct child_process cp_write_tree = CHILD_PROCESS_INIT;
+ struct index_state istate = { NULL };
cp_upd_index.git_cmd = 1;
argv_array_pushl(_upd_index.args, "update-index", "-z", "--add",
@@ -957,15 +956,11 @@ static int save_untracked_files(struct stash_info *info,
struct strbuf *msg,
goto done;
}
- cp_write_tree.git_cmd = 1;
- argv_array_push(_write_tree.args, "write-tree");
- argv_array_pushf(_write_tree.env_array, "GIT_INDEX_FILE=%s",
-stash_index_path.buf);
- if (pipe_command(_write_tree, NULL, 0, , 0,NULL, 0)) {
+ if (write_index_as_tree(>u_tree, , stash_index_path.buf, 0,
+ NULL)) {
ret = -1;
goto done;
}
- get_oid_hex(out.buf, >u_tree);
if (commit_tree(untracked_msg.buf, untracked_msg.len,
>u_tree, NULL, >u_commit, NULL, NULL)) {
@@ -974,8 +969,8 @@ static int save_untracked_files(struct stash_info *info,
struct strbuf *msg,
}
done:
+ discard_index();
strbuf_release(_msg);
- strbuf_release();
remove_path(stash_index_path.buf);
return ret;
}
@@ -984,11 +979,10 @@ static int stash_patch(struct stash_info *info, struct
pathspec ps,
struct strbuf *out_patch, int quiet)
{
int ret = 0;
- struct strbuf out = STRBUF_INIT;
struct child_process cp_read_tree = CHILD_PROCESS_INIT;
struct child_process cp_add_i = CHILD_PROCESS_INIT;
- struct child_process cp_write_tree = CHILD_PROCESS_INIT;
struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
+ struct index_state istate = { NULL };
remove_path(stash_index_path.buf);
@@ -1014,17 +1008,12 @@ static int stash_patch(struct stash_info *info, struct
pathspec ps,
}
/* State of the working tree. */
- cp_write_tree.git_cmd = 1;
- argv_array_push(_write_tree.args, "write-tree");
- argv_array_pushf(_write_tree.env_array, "GIT_INDEX_FILE=%s",
-stash_index_path.buf);
- if (pipe_command(_write_tree, NULL, 0, , 0,NULL, 0)) {
+ if (write_index_as_tree(>w_tree, , stash_index_path.buf, 0,
+ NULL)) {
ret = -1;
goto done;
}
- get_oid_hex(out.buf, >w_tree);
-
cp_diff_tree.git_cmd = 1;
argv_array_pushl(_diff_tree.args, "diff-tree", "-p", "HEAD",
oid_to_hex(>w_tree), "--", NULL);
@@ -1040,7 +1029,7 @@ static int stash_patch(struct stash_info *info, struct
pathspec ps,
}
done:
- strbuf_release();
+ discard_index();
remove_path(stash_index_path.buf);
return ret;
}
@@ -1050,9 +1039,8 @@ static int stash_working_tree(struct stash_info *info,
struct pathspec ps)
int ret = 0;
struct rev_info rev;
struct child_process cp_upd_index = CHILD_PROCESS_INIT;
- struct child_process cp_write_tree = CHILD_PROCESS_INIT;
- struct strbuf out = STRBUF_INIT;
struct strbuf diff_output = STRBUF_INIT;
+ struct index_state istate = { NULL };
set_alternate_index_output(stash_index_path.buf);
if (reset_tree(>i_tree, 0, 0)) {
@@ -1091,20 +1079,15 @@ static int stash_working_tree(struct stash_info *info,
struct pathspec ps)
goto done;
}
- cp_write_tree.git_cmd = 1;
- argv_array_push(_write_tree.args, "write-tree");
- argv_array_pushf(_write_tree.env_array, "GIT_INDEX_FILE=%s",
-stash_index_path.buf);
- if (pipe_command(_write_tree, NULL, 0, , 0,NULL, 0)) {
+ if (write_index_as_tree(>w_tree, , stash_index_path.buf, 0,
+ NULL)) {
ret = -1;
goto done;
}
- get_oid_hex(out.buf, >w_tree);
-
done:
+ discard_index();
UNLEAK(rev);
- strbuf_release();
object_array_clear();
strbuf_release(_output);
remove_path(stash_index_path.buf);
--
2.19.0.rc0.23.g1fb9f40d88
[PATCH v9 20/21] stash: optimize `get_untracked_files()` and `check_changes()`
This commits introduces a optimization by avoiding calling the
same functions again. For example, `git stash push -u`
would call at some points the following functions:
* `check_changes()` (inside `do_push_stash()`)
* `do_create_stash()`, which calls: `check_changes()` and
`get_untracked_files()`
Note that `check_changes()` also calls `get_untracked_files()`.
So, `check_changes()` is called 2 times and `get_untracked_files()`
3 times.
The old function `check_changes()` now consists of two functions:
`get_untracked_files()` and `check_changes_tracked_files()`.
These are the call chains for `push` and `create`:
* `push_stash()` -> `do_push_stash()` -> `do_create_stash()`
* `create_stash()` -> `do_create_stash()`
To prevent calling the same functions over and over again,
`check_changes()` inside `do_create_stash()` is now placed
in the caller functions (`create_stash()` and `do_push_stash()`).
This way `check_changes()` and `get_untracked files()` are called
only one time.
https://public-inbox.org/git/20180818223329.gj11...@hank.intra.tgummerer.com/
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash.c | 51 -
1 file changed, 29 insertions(+), 22 deletions(-)
diff --git a/builtin/stash.c b/builtin/stash.c
index fc4a2050b1..43d0de1f13 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -875,19 +875,18 @@ static int get_untracked_files(struct pathspec ps, int
include_untracked,
}
/*
- * The return value of `check_changes()` can be:
+ * The return value of `check_changes_tracked_files()` can be:
*
* < 0 if there was an error
* = 0 if there are no changes.
* > 0 if there are changes.
*/
-static int check_changes(struct pathspec ps, int include_untracked)
+
+static int check_changes_tracked_files(struct pathspec ps)
{
int result;
- int ret = 0;
struct rev_info rev;
struct object_id dummy;
- struct strbuf out = STRBUF_INIT;
/* No initial commit. */
if (get_oid("HEAD", ))
@@ -915,14 +914,26 @@ static int check_changes(struct pathspec ps, int
include_untracked)
if (diff_result_code(, result))
return 1;
+ return 0;
+}
+
+/*
+ * The function will fill `untracked_files` with the names of untracked files
+ * It will return 1 if there were any changes and 0 if there were not.
+ */
+
+static int check_changes(struct pathspec ps, int include_untracked,
+struct strbuf *untracked_files)
+{
+ int ret = 0;
+ if (check_changes_tracked_files(ps))
+ ret = 1;
+
if (include_untracked && get_untracked_files(ps, include_untracked,
-)) {
- strbuf_release();
- return 1;
- }
+untracked_files))
+ ret = 1;
- strbuf_release();
- return 0;
+ return ret;
}
static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
@@ -1131,7 +1142,7 @@ static int do_create_stash(struct pathspec ps, char
**stash_msg,
head_commit = lookup_commit(the_repository, >b_commit);
}
- if (!check_changes(ps, include_untracked)) {
+ if (!check_changes(ps, include_untracked, _files)) {
ret = 1;
*stash_msg = NULL;
goto done;
@@ -1157,8 +1168,7 @@ static int do_create_stash(struct pathspec ps, char
**stash_msg,
goto done;
}
- if (include_untracked && get_untracked_files(ps, include_untracked,
-_files)) {
+ if (include_untracked) {
if (save_untracked_files(info, , untracked_files)) {
if (!quiet)
fprintf_ln(stderr, _("Cannot save the untracked
files"));
@@ -1234,18 +1244,14 @@ static int create_stash(int argc, const char **argv,
const char *prefix)
++argv, ' ');
memset(, 0, sizeof(ps));
- ret = do_create_stash(ps, _msg, 0, 0, , NULL, 0);
+ if (!check_changes_tracked_files(ps))
+ return 0;
- if (!ret)
+ if (!(ret = do_create_stash(ps, _msg, 0, 0, , NULL, 0)))
printf_ln("%s", oid_to_hex(_commit));
strbuf_release(_msg_buf);
-
- /*
-* ret can be 1 if there were no changes. In this case, we should
-* not error out.
-*/
- return ret < 0;
+ return ret;
}
static int do_push_stash(struct pathspec ps, char *stash_msg, int quiet,
@@ -1254,6 +1260,7 @@ static int do_push_stash(struct pathspec ps, char
*stash_msg, int quiet,
int ret = 0;
struct stash_info info;
struct strbuf patch = STRBUF_INIT;
+ struct strbuf untracked_files = STRBUF_INIT;
if (patch_mode && keep_index == -1)
keep_index = 1;
@@
[PATCH v9 11/21] stash: convert list to builtin
Add stash list to the helper and delete the list_stash function
from the shell script.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 31 +++
git-stash.sh| 7 +--
2 files changed, 32 insertions(+), 6 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index c1f78d3275..9e256690ec 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,6 +12,7 @@
#include "rerere.h"
static const char * const git_stash_helper_usage[] = {
+ N_("git stash--helper list []"),
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet]
[]"),
N_("git stash--helper branch []"),
@@ -19,6 +20,11 @@ static const char * const git_stash_helper_usage[] = {
NULL
};
+static const char * const git_stash_helper_list_usage[] = {
+ N_("git stash--helper list []"),
+ NULL
+};
+
static const char * const git_stash_helper_drop_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
NULL
@@ -614,6 +620,29 @@ static int branch_stash(int argc, const char **argv, const
char *prefix)
return ret;
}
+static int list_stash(int argc, const char **argv, const char *prefix)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct option options[] = {
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_list_usage,
+PARSE_OPT_KEEP_UNKNOWN);
+
+ if (!ref_exists(ref_stash))
+ return 0;
+
+ cp.git_cmd = 1;
+ argv_array_pushl(, "log", "--format=%gd: %gs", "-g",
+"--first-parent", "-m", NULL);
+ argv_array_pushv(, argv);
+ argv_array_push(, ref_stash);
+ argv_array_push(, "--");
+ return run_command();
+}
+
int cmd_stash__helper(int argc, const char **argv, const char *prefix)
{
pid_t pid = getpid();
@@ -644,6 +673,8 @@ int cmd_stash__helper(int argc, const char **argv, const
char *prefix)
return !!pop_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "branch"))
return !!branch_stash(argc, argv, prefix);
+ else if (!strcmp(argv[0], "list"))
+ return !!list_stash(argc, argv, prefix);
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 8f2640fe90..6052441aa2 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -382,11 +382,6 @@ have_stash () {
git rev-parse --verify --quiet $ref_stash >/dev/null
}
-list_stash () {
- have_stash || return 0
- git log --format="%gd: %gs" -g --first-parent -m "$@" $ref_stash --
-}
-
show_stash () {
ALLOW_UNKNOWN_FLAGS=t
assert_stash_like "$@"
@@ -574,7 +569,7 @@ test -n "$seen_non_option" || set "push" "$@"
case "$1" in
list)
shift
- list_stash "$@"
+ git stash--helper list "$@"
;;
show)
shift
--
2.19.0.rc0.23.g1fb9f40d88
[PATCH v9 16/21] stash: convert push to builtin
Add stash push to the helper.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 244 +++-
git-stash.sh| 6 +-
2 files changed, 244 insertions(+), 6 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 49b05f2458..d79233d7ec 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -23,6 +23,9 @@ static const char * const git_stash_helper_usage[] = {
N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet]
[]"),
N_("git stash--helper branch []"),
N_("git stash--helper clear"),
+ N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
+ " [-u|--include-untracked] [-a|--all] [-m|--message
]\n"
+ " [--] [...]]"),
NULL
};
@@ -71,6 +74,13 @@ static const char * const git_stash_helper_create_usage[] = {
NULL
};
+static const char * const git_stash_helper_push_usage[] = {
+ N_("git stash--helper [push [-p|--patch] [-k|--[no-]keep-index]
[-q|--quiet]\n"
+ " [-u|--include-untracked] [-a|--all] [-m|--message
]\n"
+ " [--] [...]]"),
+ NULL
+};
+
static const char *ref_stash = "refs/stash";
static struct strbuf stash_index_path = STRBUF_INIT;
@@ -1088,7 +1098,7 @@ static int stash_working_tree(struct stash_info *info,
struct pathspec ps)
static int do_create_stash(struct pathspec ps, char **stash_msg,
int include_untracked, int patch_mode,
- struct stash_info *info)
+ struct stash_info *info, struct strbuf *patch)
{
int ret = 0;
int flags = 0;
@@ -1102,7 +1112,6 @@ static int do_create_stash(struct pathspec ps, char
**stash_msg,
struct strbuf commit_tree_label = STRBUF_INIT;
struct strbuf untracked_files = STRBUF_INIT;
struct strbuf stash_msg_buf = STRBUF_INIT;
- struct strbuf patch = STRBUF_INIT;
read_cache_preload(NULL);
refresh_cache(REFRESH_QUIET);
@@ -1152,7 +1161,7 @@ static int do_create_stash(struct pathspec ps, char
**stash_msg,
untracked_commit_option = 1;
}
if (patch_mode) {
- ret = stash_patch(info, ps, );
+ ret = stash_patch(info, ps, patch);
*stash_msg = NULL;
if (ret < 0) {
fprintf_ln(stderr, _("Cannot save the current worktree
state"));
@@ -1221,7 +1230,8 @@ static int create_stash(int argc, const char **argv,
const char *prefix)
0);
memset(, 0, sizeof(ps));
- ret = do_create_stash(ps, _msg, include_untracked, 0, );
+ ret = do_create_stash(ps, _msg, include_untracked, 0, ,
+ NULL);
if (!ret)
printf_ln("%s", oid_to_hex(_commit));
@@ -1234,6 +1244,230 @@ static int create_stash(int argc, const char **argv,
const char *prefix)
return ret < 0;
}
+static int do_push_stash(struct pathspec ps, char *stash_msg, int quiet,
+int keep_index, int patch_mode, int include_untracked)
+{
+ int ret = 0;
+ struct stash_info info;
+ struct strbuf patch = STRBUF_INIT;
+
+ if (patch_mode && keep_index == -1)
+ keep_index = 1;
+
+ if (patch_mode && include_untracked) {
+ fprintf_ln(stderr, _("Can't use --patch and --include-untracked"
+" or --all at the same time"));
+ ret = -1;
+ goto done;
+ }
+
+ read_cache_preload(NULL);
+ if (!include_untracked && ps.nr) {
+ int i;
+ char *ps_matched = xcalloc(ps.nr, 1);
+
+ for (i = 0; i < active_nr; i++)
+ ce_path_match(_index, active_cache[i], ,
+ ps_matched);
+
+ if (report_path_error(ps_matched, , NULL)) {
+ fprintf_ln(stderr, _("Did you forget to 'git add'?"));
+ stash_msg = NULL;
+ ret = -1;
+ goto done;
+ }
+ free(ps_matched);
+ }
+
+ if (refresh_cache(REFRESH_QUIET)) {
+ stash_msg = NULL;
+ ret = -1;
+ goto done;
+ }
+
+ if (!check_changes(ps, include_untracked)) {
+ stash_msg = NULL;
+ if (!quiet)
+ printf_ln(_("No local changes to save"));
+ goto done;
+ }
+
+ if (!reflog_exists(ref_stash) && do_clear_stash()) {
+ stash_msg = NULL;
+ ret = -1;
+ fprintf_ln(stderr, _("Cannot initialize stash"));
+ goto done;
+ }
+
+ if (do_create_stash(ps, _msg, include_untracked, patch_mode,
+ , )) {
+
[PATCH v9 00/21] Convert "git stash" to C builtin
Hello, This is a new iteration of `git stash`, based on the last review I got. This new iteration brings mostly code styling fix issues in order to make the code more readable. There is also a new patch "strbuf.c: add `strbuf_join_argv()`". By making some small changes, the code is now a little bit closer to be used as API. Joel Teichroeb (5): stash: improve option parsing test coverage stash: convert apply to builtin stash: convert drop and clear to builtin stash: convert branch to builtin stash: convert pop to builtin Paul-Sebastian Ungureanu (16): sha1-name.c: add `get_oidf()` which acts like `get_oid()` strbuf.c: add `strbuf_join_argv()` stash: update test cases conform to coding guidelines stash: rename test cases to be more descriptive stash: add tests for `git stash show` config stash: convert list to builtin stash: convert show to builtin stash: mention options in `show` synopsis. stash: convert store to builtin stash: convert create to builtin stash: convert push to builtin stash: make push -q quiet stash: convert save to builtin stash: convert `stash--helper.c` into `stash.c` stash: optimize `get_untracked_files()` and `check_changes()` stash: replace all `write-tree` child processes with API calls Documentation/git-stash.txt |4 +- Makefile |2 +- builtin.h|1 + builtin/stash.c | 1595 ++ cache.h |1 + git-stash.sh | 752 git.c|1 + sha1-name.c | 19 + strbuf.c | 15 + strbuf.h |7 + t/t3903-stash.sh | 192 ++-- t/t3907-stash-show-config.sh | 83 ++ 12 files changed, 1851 insertions(+), 821 deletions(-) create mode 100644 builtin/stash.c delete mode 100755 git-stash.sh create mode 100755 t/t3907-stash-show-config.sh -- 2.19.0.rc0.23.g1fb9f40d88
[GSoC][PATCH v9 05/21] stash: add tests for `git stash show` config
This commit introduces tests for `git stash show`
config. It tests all the cases where `stash.showStat`
and `stash.showPatch` are unset or set to true / false.
Signed-off-by: Paul-Sebastian Ungureanu
---
t/t3907-stash-show-config.sh | 83
1 file changed, 83 insertions(+)
create mode 100755 t/t3907-stash-show-config.sh
diff --git a/t/t3907-stash-show-config.sh b/t/t3907-stash-show-config.sh
new file mode 100755
index 00..10914bba7b
--- /dev/null
+++ b/t/t3907-stash-show-config.sh
@@ -0,0 +1,83 @@
+#!/bin/sh
+
+test_description='Test git stash show configuration.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ test_commit file
+'
+
+# takes three parameters:
+# 1. the stash.showStat value (or "")
+# 2. the stash.showPatch value (or "")
+# 3. the diff options of the expected output (or nothing for no output)
+test_stat_and_patch () {
+ if test "" = "$1"
+ then
+ test_unconfig stash.showStat
+ else
+ test_config stash.showStat "$1"
+ fi &&
+
+ if test "" = "$2"
+ then
+ test_unconfig stash.showPatch
+ else
+ test_config stash.showPatch "$2"
+ fi &&
+
+ shift 2 &&
+ echo 2 >file.t &&
+ if test $# != 0
+ then
+ git diff "$@" >expect
+ fi &&
+ git stash &&
+ git stash show >actual &&
+
+ if test $# = 0
+ then
+ test_must_be_empty actual
+ else
+ test_cmp expect actual
+ fi
+}
+
+test_expect_success 'showStat unset showPatch unset' '
+ test_stat_and_patch "" "" --stat
+'
+
+test_expect_success 'showStat unset showPatch false' '
+ test_stat_and_patch "" false --stat
+'
+
+test_expect_success 'showStat unset showPatch true' '
+ test_stat_and_patch "" true --stat -p
+'
+
+test_expect_success 'showStat false showPatch unset' '
+ test_stat_and_patch false ""
+'
+
+test_expect_success 'showStat false showPatch false' '
+ test_stat_and_patch false false
+'
+
+test_expect_success 'showStat false showPatch true' '
+ test_stat_and_patch false true -p
+'
+
+test_expect_success 'showStat true showPatch unset' '
+ test_stat_and_patch true "" --stat
+'
+
+test_expect_success 'showStat true showPatch false' '
+ test_stat_and_patch true false --stat
+'
+
+test_expect_success 'showStat true showPatch true' '
+ test_stat_and_patch true true --stat -p
+'
+
+test_done
--
2.19.0.rc0.23.g1c0a08a5d3
[PATCH v9 13/21] stash: mention options in `show` synopsis.
Mention in the usage text and in the documentation, that `show`
accepts any option known to `git diff`.
Signed-off-by: Paul-Sebastian Ungureanu
---
Documentation/git-stash.txt | 4 ++--
builtin/stash--helper.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 7ef8c47911..e31ea7d303 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -9,7 +9,7 @@ SYNOPSIS
[verse]
'git stash' list []
-'git stash' show []
+'git stash' show [] []
'git stash' drop [-q|--quiet] []
'git stash' ( pop | apply ) [--index] [-q|--quiet] []
'git stash' branch []
@@ -106,7 +106,7 @@ stash@{1}: On master: 9cc0589... Add git-stash
The command takes options applicable to the 'git log'
command to control what is shown and how. See linkgit:git-log[1].
-show []::
+show [] []::
Show the changes recorded in the stash entry as a diff between the
stashed contents and the commit back when the stash entry was first
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 1bc838ee6b..1f02f5f2e9 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -15,7 +15,7 @@
static const char * const git_stash_helper_usage[] = {
N_("git stash--helper list []"),
- N_("git stash--helper show []"),
+ N_("git stash--helper show [] []"),
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet]
[]"),
N_("git stash--helper branch []"),
@@ -29,7 +29,7 @@ static const char * const git_stash_helper_list_usage[] = {
};
static const char * const git_stash_helper_show_usage[] = {
- N_("git stash--helper show []"),
+ N_("git stash--helper show [] []"),
NULL
};
--
2.19.0.rc0.23.g1fb9f40d88
[PATCH v9 17/21] stash: make push -q quiet
There is a change in behaviour with this commit. When there was
no initial commit, the shell version of stash would still display
a message. This commit makes `push` to not display any message if
`--quiet` or `-q` is specified.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 45 ++---
t/t3903-stash.sh| 23 +
2 files changed, 52 insertions(+), 16 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index d79233d7ec..73bb22dc94 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -967,7 +967,7 @@ static int save_untracked_files(struct stash_info *info,
struct strbuf *msg,
}
static int stash_patch(struct stash_info *info, struct pathspec ps,
- struct strbuf *out_patch)
+ struct strbuf *out_patch, int quiet)
{
int ret = 0;
struct strbuf out = STRBUF_INIT;
@@ -1020,7 +1020,8 @@ static int stash_patch(struct stash_info *info, struct
pathspec ps,
}
if (!out_patch->len) {
- fprintf_ln(stderr, _("No changes selected"));
+ if (!quiet)
+ fprintf_ln(stderr, _("No changes selected"));
ret = 1;
}
@@ -1098,7 +1099,8 @@ static int stash_working_tree(struct stash_info *info,
struct pathspec ps)
static int do_create_stash(struct pathspec ps, char **stash_msg,
int include_untracked, int patch_mode,
- struct stash_info *info, struct strbuf *patch)
+ struct stash_info *info, struct strbuf *patch,
+ int quiet)
{
int ret = 0;
int flags = 0;
@@ -1117,7 +1119,8 @@ static int do_create_stash(struct pathspec ps, char
**stash_msg,
refresh_cache(REFRESH_QUIET);
if (get_oid("HEAD", >b_commit)) {
- fprintf_ln(stderr, _("You do not have the initial commit yet"));
+ if (!quiet)
+ fprintf_ln(stderr, _("You do not have the initial
commit yet"));
ret = -1;
*stash_msg = NULL;
goto done;
@@ -1144,7 +1147,8 @@ static int do_create_stash(struct pathspec ps, char
**stash_msg,
if (write_cache_as_tree(>i_tree, 0, NULL) ||
commit_tree(commit_tree_label.buf, commit_tree_label.len,
>i_tree, parents, >i_commit, NULL, NULL)) {
- fprintf_ln(stderr, _("Cannot save the current index state"));
+ if (!quiet)
+ fprintf_ln(stderr, _("Cannot save the current index
state"));
ret = -1;
*stash_msg = NULL;
goto done;
@@ -1153,7 +1157,8 @@ static int do_create_stash(struct pathspec ps, char
**stash_msg,
if (include_untracked && get_untracked_files(ps, include_untracked,
_files)) {
if (save_untracked_files(info, , untracked_files)) {
- fprintf_ln(stderr, _("Cannot save the untracked
files"));
+ if (!quiet)
+ fprintf_ln(stderr, _("Cannot save the untracked
files"));
ret = -1;
*stash_msg = NULL;
goto done;
@@ -1161,17 +1166,19 @@ static int do_create_stash(struct pathspec ps, char
**stash_msg,
untracked_commit_option = 1;
}
if (patch_mode) {
- ret = stash_patch(info, ps, patch);
+ ret = stash_patch(info, ps, patch, quiet);
*stash_msg = NULL;
if (ret < 0) {
- fprintf_ln(stderr, _("Cannot save the current worktree
state"));
+ if (!quiet)
+ fprintf_ln(stderr, _("Cannot save the current
worktree state"));
goto done;
} else if (ret > 0) {
goto done;
}
} else {
if (stash_working_tree(info, ps)) {
- fprintf_ln(stderr, _("Cannot save the current worktree
state"));
+ if (!quiet)
+ fprintf_ln(stderr, _("Cannot save the current
worktree state"));
ret = -1;
*stash_msg = NULL;
goto done;
@@ -1197,7 +1204,8 @@ static int do_create_stash(struct pathspec ps, char
**stash_msg,
if (commit_tree(*stash_msg, strlen(*stash_msg), >w_tree,
parents, >w_commit, NULL, NULL)) {
- fprintf_ln(stderr, _("Cannot record working tree state"));
+ if (!quiet)
+ fprintf_ln(stderr, _("Cannot record working tree
state"));
ret = -1;
goto done;
}
@@ -1231,7 +1239,7 @@ static int
[PATCH v9 09/21] stash: convert branch to builtin
From: Joel Teichroeb
Add stash branch to the helper and delete the apply_to_branch
function from the shell script.
Checkout does not currently provide a function for checking out
a branch as cmd_checkout does a large amount of sanity checks
first that we require here.
Signed-off-by: Joel Teichroeb
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 46 +
git-stash.sh| 17 ++-
2 files changed, 48 insertions(+), 15 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 72472eaeb7..5841bd0e98 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -14,6 +14,7 @@
static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+ N_("git stash--helper branch []"),
N_("git stash--helper clear"),
NULL
};
@@ -28,6 +29,11 @@ static const char * const git_stash_helper_apply_usage[] = {
NULL
};
+static const char * const git_stash_helper_branch_usage[] = {
+ N_("git stash--helper branch []"),
+ NULL
+};
+
static const char * const git_stash_helper_clear_usage[] = {
N_("git stash--helper clear"),
NULL
@@ -536,6 +542,44 @@ static int drop_stash(int argc, const char **argv, const
char *prefix)
return ret;
}
+static int branch_stash(int argc, const char **argv, const char *prefix)
+{
+ int ret;
+ const char *branch = NULL;
+ struct stash_info info;
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct option options[] = {
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, prefix, options,
+git_stash_helper_branch_usage, 0);
+
+ if (!argc) {
+ fprintf_ln(stderr, "No branch name specified");
+ return -1;
+ }
+
+ branch = argv[0];
+
+ if (get_stash_info(, argc - 1, argv + 1))
+ return -1;
+
+ cp.git_cmd = 1;
+ argv_array_pushl(, "checkout", "-b", NULL);
+ argv_array_push(, branch);
+ argv_array_push(, oid_to_hex(_commit));
+ ret = run_command();
+ if (!ret)
+ ret = do_apply_stash(prefix, , 1, 0);
+ if (!ret && info.is_stash_ref)
+ ret = do_drop_stash(prefix, , 0);
+
+ free_stash_info();
+
+ return ret;
+}
+
int cmd_stash__helper(int argc, const char **argv, const char *prefix)
{
pid_t pid = getpid();
@@ -562,6 +606,8 @@ int cmd_stash__helper(int argc, const char **argv, const
char *prefix)
return !!clear_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "drop"))
return !!drop_stash(argc, argv, prefix);
+ else if (!strcmp(argv[0], "branch"))
+ return !!branch_stash(argc, argv, prefix);
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index a99d5dc9e5..29d9f44255 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -598,20 +598,6 @@ drop_stash () {
clear_stash
}
-apply_to_branch () {
- test -n "$1" || die "$(gettext "No branch name specified")"
- branch=$1
- shift 1
-
- set -- --index "$@"
- assert_stash_like "$@"
-
- git checkout -b $branch $REV^ &&
- apply_stash "$@" && {
- test -z "$IS_STASH_REF" || drop_stash "$@"
- }
-}
-
test "$1" = "-p" && set "push" "$@"
PARSE_CACHE='--not-parsed'
@@ -673,7 +659,8 @@ pop)
;;
branch)
shift
- apply_to_branch "$@"
+ cd "$START_DIR"
+ git stash--helper branch "$@"
;;
*)
case $# in
--
2.19.0.rc0.23.g1fb9f40d88
[PATCH v9 04/21] stash: update test cases conform to coding guidelines
Removed whitespaces after redirection operators.
Signed-off-by: Paul-Sebastian Ungureanu
---
t/t3903-stash.sh | 120 ---
1 file changed, 61 insertions(+), 59 deletions(-)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index af7586d43d..de6cab1fe7 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -8,22 +8,22 @@ test_description='Test git stash'
. ./test-lib.sh
test_expect_success 'stash some dirty working directory' '
- echo 1 > file &&
+ echo 1 >file &&
git add file &&
echo unrelated >other-file &&
git add other-file &&
test_tick &&
git commit -m initial &&
- echo 2 > file &&
+ echo 2 >file &&
git add file &&
- echo 3 > file &&
+ echo 3 >file &&
test_tick &&
git stash &&
git diff-files --quiet &&
git diff-index --cached --quiet HEAD
'
-cat > expect << EOF
+cat >expect < output &&
+ git diff stash^2..stash >output &&
test_cmp output expect
'
@@ -74,7 +74,7 @@ test_expect_success 'apply stashed changes' '
test_expect_success 'apply stashed changes (including index)' '
git reset --hard HEAD^ &&
- echo 6 > other-file &&
+ echo 6 >other-file &&
git add other-file &&
test_tick &&
git commit -m other-file &&
@@ -99,12 +99,12 @@ test_expect_success 'stash drop complains of extra options'
'
test_expect_success 'drop top stash' '
git reset --hard &&
- git stash list > stashlist1 &&
- echo 7 > file &&
+ git stash list >expected &&
+ echo 7 >file &&
git stash &&
git stash drop &&
- git stash list > stashlist2 &&
- test_cmp stashlist1 stashlist2 &&
+ git stash list >actual &&
+ test_cmp expected actual &&
git stash apply &&
test 3 = $(cat file) &&
test 1 = $(git show :file) &&
@@ -113,9 +113,9 @@ test_expect_success 'drop top stash' '
test_expect_success 'drop middle stash' '
git reset --hard &&
- echo 8 > file &&
+ echo 8 >file &&
git stash &&
- echo 9 > file &&
+ echo 9 >file &&
git stash &&
git stash drop stash@{1} &&
test 2 = $(git stash list | wc -l) &&
@@ -160,7 +160,7 @@ test_expect_success 'stash pop' '
test 0 = $(git stash list | wc -l)
'
-cat > expect << EOF
+cat >expect < expect1 << EOF
+cat >expect1 < expect2 << EOF
+cat >expect2 < file &&
+ echo foo >file &&
git commit file -m first &&
- echo bar > file &&
- echo bar2 > file2 &&
+ echo bar >file &&
+ echo bar2 >file2 &&
git add file2 &&
git stash &&
- echo baz > file &&
+ echo baz >file &&
git commit file -m second &&
git stash branch stashbranch &&
test refs/heads/stashbranch = $(git symbolic-ref HEAD) &&
test $(git rev-parse HEAD) = $(git rev-parse master^) &&
- git diff --cached > output &&
+ git diff --cached >output &&
test_cmp output expect &&
- git diff > output &&
+ git diff >output &&
test_cmp output expect1 &&
git add file &&
git commit -m alternate\ second &&
- git diff master..stashbranch > output &&
+ git diff master..stashbranch >output &&
test_cmp output expect2 &&
test 0 = $(git stash list | wc -l)
'
test_expect_success 'apply -q is quiet' '
- echo foo > file &&
+ echo foo >file &&
git stash &&
- git stash apply -q > output.out 2>&1 &&
+ git stash apply -q >output.out 2>&1 &&
test_must_be_empty output.out
'
test_expect_success 'save -q is quiet' '
- git stash save --quiet > output.out 2>&1 &&
+ git stash save --quiet >output.out 2>&1 &&
test_must_be_empty output.out
'
test_expect_success 'pop -q is quiet' '
- git stash pop -q > output.out 2>&1 &&
+ git stash pop -q >output.out 2>&1 &&
test_must_be_empty output.out
'
test_expect_success 'pop -q --index works and is quiet' '
- echo foo > file &&
+ echo foo >file &&
git add file &&
git stash save --quiet &&
- git stash pop -q --index > output.out 2>&1 &&
+ git stash pop -q --index >output.out 2>&1 &&
test foo = "$(git show :file)" &&
test_must_be_empty output.out
'
test_expect_success 'drop -q is quiet' '
git stash &&
- git stash drop -q > output.out 2>&1 &&
+ git stash drop -q >output.out 2>&1 &&
test_must_be_empty output.out
'
test_expect_success 'stash -k' '
- echo bar3 > file &&
- echo bar4 > file2 &&
+ echo bar3 >file &&
+ echo bar4 >file2 &&
git add file2 &&
git stash -k &&
test bar,bar4 = $(cat file),$(cat file2)
'
test_expect_success 'stash --no-keep-index' '
- echo bar33 > file &&
- echo bar44 > file2 &&
+ echo bar33 >file &&
+ echo
[PATCH v9 12/21] stash: convert show to builtin
Add stash show to the helper and delete the show_stash, have_stash,
assert_stash_like, is_stash_like and parse_flags_and_rev functions
from the shell script now that they are no longer needed.
In shell version, although `git stash show` accepts `--index` and
`--quiet` options, it ignores them. In C, both options are passed
further to `git diff`.
Signed-off-by: Paul-Sebastian Ungureanu
---
builtin/stash--helper.c | 87 ++
git-stash.sh| 132 +---
2 files changed, 88 insertions(+), 131 deletions(-)
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 9e256690ec..1bc838ee6b 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -10,9 +10,12 @@
#include "run-command.h"
#include "dir.h"
#include "rerere.h"
+#include "revision.h"
+#include "log-tree.h"
static const char * const git_stash_helper_usage[] = {
N_("git stash--helper list []"),
+ N_("git stash--helper show []"),
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet]
[]"),
N_("git stash--helper branch []"),
@@ -25,6 +28,11 @@ static const char * const git_stash_helper_list_usage[] = {
NULL
};
+static const char * const git_stash_helper_show_usage[] = {
+ N_("git stash--helper show []"),
+ NULL
+};
+
static const char * const git_stash_helper_drop_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
NULL
@@ -643,6 +651,83 @@ static int list_stash(int argc, const char **argv, const
char *prefix)
return run_command();
}
+static int show_stat = 1;
+static int show_patch;
+
+static int git_stash_config(const char *var, const char *value, void *cb)
+{
+ if (!strcmp(var, "stash.showstat")) {
+ show_stat = git_config_bool(var, value);
+ return 0;
+ }
+ if (!strcmp(var, "stash.showpatch")) {
+ show_patch = git_config_bool(var, value);
+ return 0;
+ }
+ return git_default_config(var, value, cb);
+}
+
+static int show_stash(int argc, const char **argv, const char *prefix)
+{
+ int i;
+ int opts = 0;
+ int ret = 0;
+ struct stash_info info;
+ struct rev_info rev;
+ struct argv_array stash_args = ARGV_ARRAY_INIT;
+ struct option options[] = {
+ OPT_END()
+ };
+
+ init_diff_ui_defaults();
+ git_config(git_diff_ui_config, NULL);
+ init_revisions(, prefix);
+
+ for (i = 1; i < argc; i++) {
+ if (argv[i][0] != '-')
+ argv_array_push(_args, argv[i]);
+ else
+ opts++;
+ }
+
+ ret = get_stash_info(, stash_args.argc, stash_args.argv);
+ argv_array_clear(_args);
+ if (ret)
+ return -1;
+
+ /*
+* The config settings are applied only if there are not passed
+* any options.
+*/
+ if (!opts) {
+ git_config(git_stash_config, NULL);
+ if (show_stat)
+ rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT;
+
+ if (show_patch)
+ rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
+
+ if (!show_stat && !show_patch) {
+ free_stash_info();
+ return 0;
+ }
+ }
+
+ argc = setup_revisions(argc, argv, , NULL);
+ if (argc > 1) {
+ free_stash_info();
+ usage_with_options(git_stash_helper_show_usage, options);
+ }
+
+ rev.diffopt.flags.recursive = 1;
+ setup_diff_pager();
+ diff_tree_oid(_commit, _commit, "", );
+ log_tree_diff_flush();
+
+ free_stash_info();
+ return diff_result_code(, 0);
+}
+
int cmd_stash__helper(int argc, const char **argv, const char *prefix)
{
pid_t pid = getpid();
@@ -675,6 +760,8 @@ int cmd_stash__helper(int argc, const char **argv, const
char *prefix)
return !!branch_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "list"))
return !!list_stash(argc, argv, prefix);
+ else if (!strcmp(argv[0], "show"))
+ return !!show_stash(argc, argv, prefix);
usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 6052441aa2..0d05cbc1e5 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -378,35 +378,6 @@ save_stash () {
fi
}
-have_stash () {
- git rev-parse --verify --quiet $ref_stash >/dev/null
-}
-
-show_stash () {
- ALLOW_UNKNOWN_FLAGS=t
- assert_stash_like "$@"
-
- if test -z "$FLAGS"
- then
- if test "$(git config --bool stash.showStat || echo true)" =
"true"
- then
- FLAGS=--stat
-
[PATCH v9 02/21] strbuf.c: add `strbuf_join_argv()`
Implement `strbuf_join_argv()` to join arguments
into a strbuf.
Signed-off-by: Paul-Sebastian Ungureanu
---
strbuf.c | 15 +++
strbuf.h | 7 +++
2 files changed, 22 insertions(+)
diff --git a/strbuf.c b/strbuf.c
index 64041c3c24..3eb431b2b0 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -259,6 +259,21 @@ void strbuf_addbuf(struct strbuf *sb, const struct strbuf
*sb2)
strbuf_setlen(sb, sb->len + sb2->len);
}
+const char *strbuf_join_argv(struct strbuf *buf,
+int argc, const char **argv, char delim)
+{
+ if (!argc)
+ return buf->buf;
+
+ strbuf_addstr(buf, *argv);
+ while (--argc) {
+ strbuf_addch(buf, delim);
+ strbuf_addstr(buf, *(++argv));
+ }
+
+ return buf->buf;
+}
+
void strbuf_addchars(struct strbuf *sb, int c, size_t n)
{
strbuf_grow(sb, n);
diff --git a/strbuf.h b/strbuf.h
index 60a35aef16..7ed859bb8a 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -284,6 +284,13 @@ static inline void strbuf_addstr(struct strbuf *sb, const
char *s)
*/
extern void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2);
+
+/**
+ *
+ */
+extern const char *strbuf_join_argv(struct strbuf *buf, int argc,
+ const char **argv, char delim);
+
/**
* This function can be used to expand a format string containing
* placeholders. To that end, it parses the string and calls the specified
--
2.19.0.rc0.23.g1fb9f40d88
[GSoC][PATCH v9 04/21] stash: rename test cases to be more descriptive
Rename some test cases' labels to be more descriptive and under 80
characters per line.
Signed-off-by: Paul-Sebastian Ungureanu
---
t/t3903-stash.sh | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index de6cab1fe7..3114c7bc4c 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -604,7 +604,7 @@ test_expect_success 'stash show -p - no stashes on stack,
stash-like argument' '
test_cmp expected actual
'
-test_expect_success 'stash drop - fail early if specified stash is not a stash
reference' '
+test_expect_success 'drop: fail early if specified stash is not a stash ref' '
git stash clear &&
test_when_finished "git reset --hard HEAD && git stash clear" &&
git reset --hard &&
@@ -618,7 +618,7 @@ test_expect_success 'stash drop - fail early if specified
stash is not a stash r
git reset --hard HEAD
'
-test_expect_success 'stash pop - fail early if specified stash is not a stash
reference' '
+test_expect_success 'pop: fail early if specified stash is not a stash ref' '
git stash clear &&
test_when_finished "git reset --hard HEAD && git stash clear" &&
git reset --hard &&
@@ -682,7 +682,7 @@ test_expect_success 'invalid ref of the form "n", n >= N' '
git stash drop
'
-test_expect_success 'stash branch should not drop the stash if the branch
exists' '
+test_expect_success 'branch: do not drop the stash if the branch exists' '
git stash clear &&
echo foo >file &&
git add file &&
@@ -693,7 +693,7 @@ test_expect_success 'stash branch should not drop the stash
if the branch exists
git rev-parse stash@{0} --
'
-test_expect_success 'stash branch should not drop the stash if the apply
fails' '
+test_expect_success 'branch: should not drop the stash if the apply fails' '
git stash clear &&
git reset HEAD~1 --hard &&
echo foo >file &&
@@ -707,7 +707,7 @@ test_expect_success 'stash branch should not drop the stash
if the apply fails'
git rev-parse stash@{0} --
'
-test_expect_success 'stash apply shows status same as git status (relative to
current directory)' '
+test_expect_success 'apply: show same status as git status (relative to ./)' '
git stash clear &&
echo 1 >subdir/subfile1 &&
echo 2 >subdir/subfile2 &&
@@ -1048,7 +1048,7 @@ test_expect_success 'stash push -p with pathspec shows no
changes only once' '
test_i18ncmp expect actual
'
-test_expect_success 'stash push with pathspec shows no changes when there are
none' '
+test_expect_success 'push : show no changes when there are none' '
>foo &&
git add foo &&
git commit -m "tmp" &&
@@ -1058,7 +1058,7 @@ test_expect_success 'stash push with pathspec shows no
changes when there are no
test_i18ncmp expect actual
'
-test_expect_success 'stash push with pathspec not in the repository errors
out' '
+test_expect_success 'push: not in the repository errors out' '
>untracked &&
test_must_fail git stash push untracked &&
test_path_is_file untracked
--
2.19.0.rc0.23.g1c0a08a5d3
[PATCH v9 05/21] stash: rename test cases to be more descriptive
Rename some test cases' labels to be more descriptive and under 80
characters per line.
Signed-off-by: Paul-Sebastian Ungureanu
---
t/t3903-stash.sh | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index de6cab1fe7..3114c7bc4c 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -604,7 +604,7 @@ test_expect_success 'stash show -p - no stashes on stack,
stash-like argument' '
test_cmp expected actual
'
-test_expect_success 'stash drop - fail early if specified stash is not a stash
reference' '
+test_expect_success 'drop: fail early if specified stash is not a stash ref' '
git stash clear &&
test_when_finished "git reset --hard HEAD && git stash clear" &&
git reset --hard &&
@@ -618,7 +618,7 @@ test_expect_success 'stash drop - fail early if specified
stash is not a stash r
git reset --hard HEAD
'
-test_expect_success 'stash pop - fail early if specified stash is not a stash
reference' '
+test_expect_success 'pop: fail early if specified stash is not a stash ref' '
git stash clear &&
test_when_finished "git reset --hard HEAD && git stash clear" &&
git reset --hard &&
@@ -682,7 +682,7 @@ test_expect_success 'invalid ref of the form "n", n >= N' '
git stash drop
'
-test_expect_success 'stash branch should not drop the stash if the branch
exists' '
+test_expect_success 'branch: do not drop the stash if the branch exists' '
git stash clear &&
echo foo >file &&
git add file &&
@@ -693,7 +693,7 @@ test_expect_success 'stash branch should not drop the stash
if the branch exists
git rev-parse stash@{0} --
'
-test_expect_success 'stash branch should not drop the stash if the apply
fails' '
+test_expect_success 'branch: should not drop the stash if the apply fails' '
git stash clear &&
git reset HEAD~1 --hard &&
echo foo >file &&
@@ -707,7 +707,7 @@ test_expect_success 'stash branch should not drop the stash
if the apply fails'
git rev-parse stash@{0} --
'
-test_expect_success 'stash apply shows status same as git status (relative to
current directory)' '
+test_expect_success 'apply: show same status as git status (relative to ./)' '
git stash clear &&
echo 1 >subdir/subfile1 &&
echo 2 >subdir/subfile2 &&
@@ -1048,7 +1048,7 @@ test_expect_success 'stash push -p with pathspec shows no
changes only once' '
test_i18ncmp expect actual
'
-test_expect_success 'stash push with pathspec shows no changes when there are
none' '
+test_expect_success 'push : show no changes when there are none' '
>foo &&
git add foo &&
git commit -m "tmp" &&
@@ -1058,7 +1058,7 @@ test_expect_success 'stash push with pathspec shows no
changes when there are no
test_i18ncmp expect actual
'
-test_expect_success 'stash push with pathspec not in the repository errors
out' '
+test_expect_success 'push: not in the repository errors out' '
>untracked &&
test_must_fail git stash push untracked &&
test_path_is_file untracked
--
2.19.0.rc0.23.g1fb9f40d88
[GSoC][PATCH v9 02/21] stash: improve option parsing test coverage
From: Joel Teichroeb
In preparation for converting the stash command incrementally to
a builtin command, this patch improves test coverage of the option
parsing. Both for having too many parameters, or too few.
Signed-off-by: Joel Teichroeb
Signed-off-by: Paul-Sebastian Ungureanu
---
t/t3903-stash.sh | 35 +++
1 file changed, 35 insertions(+)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 1f871d3cca..af7586d43d 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -444,6 +444,36 @@ test_expect_failure 'stash file to directory' '
test foo = "$(cat file/file)"
'
+test_expect_success 'giving too many ref arguments does not modify files' '
+ git stash clear &&
+ test_when_finished "git reset --hard HEAD" &&
+ echo foo >file2 &&
+ git stash &&
+ echo bar >file2 &&
+ git stash &&
+ test-tool chmtime =123456789 file2 &&
+ for type in apply pop "branch stash-branch"
+ do
+ test_must_fail git stash $type stash@{0} stash@{1} 2>err &&
+ test_i18ngrep "Too many revisions" err &&
+ test 123456789 = $(test-tool chmtime -g file2) || return 1
+ done
+'
+
+test_expect_success 'drop: too many arguments errors out (does nothing)' '
+ git stash list >expect &&
+ test_must_fail git stash drop stash@{0} stash@{1} 2>err &&
+ test_i18ngrep "Too many revisions" err &&
+ git stash list >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'show: too many arguments errors out (does nothing)' '
+ test_must_fail git stash show stash@{0} stash@{1} 2>err 1>out &&
+ test_i18ngrep "Too many revisions" err &&
+ test_must_be_empty out
+'
+
test_expect_success 'stash create - no changes' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
@@ -479,6 +509,11 @@ test_expect_success 'stash branch - stashes on stack,
stash-like argument' '
test $(git ls-files --modified | wc -l) -eq 1
'
+test_expect_success 'stash branch complains with no arguments' '
+ test_must_fail git stash branch 2>err &&
+ test_i18ngrep "No branch name specified" err
+'
+
test_expect_success 'stash show format defaults to --stat' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
--
2.19.0.rc0.23.g1c0a08a5d3
[GSoC][PATCH v9 03/21] stash: update test cases conform to coding guidelines
Removed whitespaces after redirection operators.
Signed-off-by: Paul-Sebastian Ungureanu
---
t/t3903-stash.sh | 120 ---
1 file changed, 61 insertions(+), 59 deletions(-)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index af7586d43d..de6cab1fe7 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -8,22 +8,22 @@ test_description='Test git stash'
. ./test-lib.sh
test_expect_success 'stash some dirty working directory' '
- echo 1 > file &&
+ echo 1 >file &&
git add file &&
echo unrelated >other-file &&
git add other-file &&
test_tick &&
git commit -m initial &&
- echo 2 > file &&
+ echo 2 >file &&
git add file &&
- echo 3 > file &&
+ echo 3 >file &&
test_tick &&
git stash &&
git diff-files --quiet &&
git diff-index --cached --quiet HEAD
'
-cat > expect << EOF
+cat >expect < output &&
+ git diff stash^2..stash >output &&
test_cmp output expect
'
@@ -74,7 +74,7 @@ test_expect_success 'apply stashed changes' '
test_expect_success 'apply stashed changes (including index)' '
git reset --hard HEAD^ &&
- echo 6 > other-file &&
+ echo 6 >other-file &&
git add other-file &&
test_tick &&
git commit -m other-file &&
@@ -99,12 +99,12 @@ test_expect_success 'stash drop complains of extra options'
'
test_expect_success 'drop top stash' '
git reset --hard &&
- git stash list > stashlist1 &&
- echo 7 > file &&
+ git stash list >expected &&
+ echo 7 >file &&
git stash &&
git stash drop &&
- git stash list > stashlist2 &&
- test_cmp stashlist1 stashlist2 &&
+ git stash list >actual &&
+ test_cmp expected actual &&
git stash apply &&
test 3 = $(cat file) &&
test 1 = $(git show :file) &&
@@ -113,9 +113,9 @@ test_expect_success 'drop top stash' '
test_expect_success 'drop middle stash' '
git reset --hard &&
- echo 8 > file &&
+ echo 8 >file &&
git stash &&
- echo 9 > file &&
+ echo 9 >file &&
git stash &&
git stash drop stash@{1} &&
test 2 = $(git stash list | wc -l) &&
@@ -160,7 +160,7 @@ test_expect_success 'stash pop' '
test 0 = $(git stash list | wc -l)
'
-cat > expect << EOF
+cat >expect < expect1 << EOF
+cat >expect1 < expect2 << EOF
+cat >expect2 < file &&
+ echo foo >file &&
git commit file -m first &&
- echo bar > file &&
- echo bar2 > file2 &&
+ echo bar >file &&
+ echo bar2 >file2 &&
git add file2 &&
git stash &&
- echo baz > file &&
+ echo baz >file &&
git commit file -m second &&
git stash branch stashbranch &&
test refs/heads/stashbranch = $(git symbolic-ref HEAD) &&
test $(git rev-parse HEAD) = $(git rev-parse master^) &&
- git diff --cached > output &&
+ git diff --cached >output &&
test_cmp output expect &&
- git diff > output &&
+ git diff >output &&
test_cmp output expect1 &&
git add file &&
git commit -m alternate\ second &&
- git diff master..stashbranch > output &&
+ git diff master..stashbranch >output &&
test_cmp output expect2 &&
test 0 = $(git stash list | wc -l)
'
test_expect_success 'apply -q is quiet' '
- echo foo > file &&
+ echo foo >file &&
git stash &&
- git stash apply -q > output.out 2>&1 &&
+ git stash apply -q >output.out 2>&1 &&
test_must_be_empty output.out
'
test_expect_success 'save -q is quiet' '
- git stash save --quiet > output.out 2>&1 &&
+ git stash save --quiet >output.out 2>&1 &&
test_must_be_empty output.out
'
test_expect_success 'pop -q is quiet' '
- git stash pop -q > output.out 2>&1 &&
+ git stash pop -q >output.out 2>&1 &&
test_must_be_empty output.out
'
test_expect_success 'pop -q --index works and is quiet' '
- echo foo > file &&
+ echo foo >file &&
git add file &&
git stash save --quiet &&
- git stash pop -q --index > output.out 2>&1 &&
+ git stash pop -q --index >output.out 2>&1 &&
test foo = "$(git show :file)" &&
test_must_be_empty output.out
'
test_expect_success 'drop -q is quiet' '
git stash &&
- git stash drop -q > output.out 2>&1 &&
+ git stash drop -q >output.out 2>&1 &&
test_must_be_empty output.out
'
test_expect_success 'stash -k' '
- echo bar3 > file &&
- echo bar4 > file2 &&
+ echo bar3 >file &&
+ echo bar4 >file2 &&
git add file2 &&
git stash -k &&
test bar,bar4 = $(cat file),$(cat file2)
'
test_expect_success 'stash --no-keep-index' '
- echo bar33 > file &&
- echo bar44 > file2 &&
+ echo bar33 >file &&
+ echo
[PATCH v9 03/21] stash: improve option parsing test coverage
From: Joel Teichroeb
In preparation for converting the stash command incrementally to
a builtin command, this patch improves test coverage of the option
parsing. Both for having too many parameters, or too few.
Signed-off-by: Joel Teichroeb
Signed-off-by: Paul-Sebastian Ungureanu
---
t/t3903-stash.sh | 35 +++
1 file changed, 35 insertions(+)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 1f871d3cca..af7586d43d 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -444,6 +444,36 @@ test_expect_failure 'stash file to directory' '
test foo = "$(cat file/file)"
'
+test_expect_success 'giving too many ref arguments does not modify files' '
+ git stash clear &&
+ test_when_finished "git reset --hard HEAD" &&
+ echo foo >file2 &&
+ git stash &&
+ echo bar >file2 &&
+ git stash &&
+ test-tool chmtime =123456789 file2 &&
+ for type in apply pop "branch stash-branch"
+ do
+ test_must_fail git stash $type stash@{0} stash@{1} 2>err &&
+ test_i18ngrep "Too many revisions" err &&
+ test 123456789 = $(test-tool chmtime -g file2) || return 1
+ done
+'
+
+test_expect_success 'drop: too many arguments errors out (does nothing)' '
+ git stash list >expect &&
+ test_must_fail git stash drop stash@{0} stash@{1} 2>err &&
+ test_i18ngrep "Too many revisions" err &&
+ git stash list >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'show: too many arguments errors out (does nothing)' '
+ test_must_fail git stash show stash@{0} stash@{1} 2>err 1>out &&
+ test_i18ngrep "Too many revisions" err &&
+ test_must_be_empty out
+'
+
test_expect_success 'stash create - no changes' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
@@ -479,6 +509,11 @@ test_expect_success 'stash branch - stashes on stack,
stash-like argument' '
test $(git ls-files --modified | wc -l) -eq 1
'
+test_expect_success 'stash branch complains with no arguments' '
+ test_must_fail git stash branch 2>err &&
+ test_i18ngrep "No branch name specified" err
+'
+
test_expect_success 'stash show format defaults to --stat' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
--
2.19.0.rc0.23.g1fb9f40d88
Re: [PATCH v4 9/9] Documentation/config: add odb..promisorRemote
Christian Couder writes: > The main issue that this patch series tries to solve is that > extensions.partialclone config option limits the partial clone and > promisor features to only one remote. One related issue is that it > also prevents to have other kind of promisor/partial clone/odb > remotes. By other kind I mean remotes that would not necessarily be > git repos, but that could store objects (that's where ODB, for Object > DataBase, comes from) and could provide those objects to Git through a > helper (or driver) script or program. I do not think "sources that are not git repositories" is all that interesting, unless they can also serve as the source for ext:: remote helper. And if they can serve "git fetch ext::...", I think they can be treated just like a normal Git repository by the backfill code that needs to lazily populate the partial clone. And it would be nice to be able to say "I took these commits from that remote and that remote should be able to backfill the trees and the blobs necessary to complete these commits" for more than one remote would obviously be a good thing. The way we mark the promisor packs currently is by a mere presence of a file, but nothing prevents us from extending it to write the nickname of the configured remote the pack was taken from to help us answer "who can feed us the remaining objects?", for example, so I do not think it is an insurmountable problem I guess JTan is the primary person who is interested/working on the partial clone with backfill? Have you two been collaborating well? Do you two need help from us to make that happen, and if so what do you need? Stop the world and declare this and that source files are off limits for two weeks, or something like that?
Re: [GSoC][PATCH v8 18/20] stash: convert `stash--helper.c` into `stash.c`
Hi,
@@ -1443,9 +1448,10 @@ static int push_stash(int argc, const char **argv, const char *prefix)
OPT_END()
};
- argc = parse_options(argc, argv, prefix, options,
-git_stash_helper_push_usage,
-0);
+ if (argc)
+ argc = parse_options(argc, argv, prefix, options,
+git_stash_push_usage,
+0);
Is this `if (argc)` guard necessary?
Yes because without it, there would be a seg fault when
calling `git stash`. Why?
After spawning `git stash`, in `push_stash()`: `argc` would be
0 (and `argv` would be `NULL`).
`parse_options()` calls `parse_options_start()` which does the following:
ctx->argc = ctx->total = argc - 1;
ctx->argv = argv + 1;
So, `ctx->argc` would be `-1`. After we are back to `parse_options()`,
`parse_options_step()` would be called. In `parse_options_step()`:
for (; ctx->argc; ctx->argc--, ctx->argv++)
Which is an infinite loop, because `ctx->argc` is already -1.
This check is also done for `git notes list` (function `list()`
inside `builtin/notes.c`). Now, that I relook at it, it seems to me
that this is a bug. Probably a better solution would be to check at the
beginning of `parse_options()` and return early if `argc` is less then one.
@@ -1536,7 +1544,44 @@ int cmd_stash__helper(int argc, const char **argv, const
char *prefix)
return !!push_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "save"))
return !!save_stash(argc, argv, prefix);
+ else if (*argv[0] != '-')
+ usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
+ git_stash_usage, options);
+
+ if (strcmp(argv[0], "-p")) {
+ while (++i < argc && strcmp(argv[i], "--")) {
+ /*
+* `akpqu` is a string which contains all short options,
+* except `-m` which is verified separately.
+*/
+ if ((strlen(argv[i]) == 2) && *argv[i] == '-' &&
+ strchr("akpqu", argv[i][1]))
I *think* this is missing the `n`.
I guess that by `n` you are referring to the short option of
`--no-keep-index`, which is missing because it was also omitted
in `stash.sh`. I am not sure whether it is worth adding it. In
case `stash` will learn any other option starting with `n`, this
might create confusion amongst users.
Best regards,
Paul
Re: [GSoC][PATCH v8 14/20] stash: convert create to builtin
Hi, Sorry for the late reply. I had a lot on my plate for the last couple of weeks. + + git_config(git_diff_basic_config, NULL); Is this not called in as part of `git_config(git_default_config, NULL);` in cmd_stash() already? *clicketyclick* I guess not. But then, maybe it would make sense to run with `git_diff_basic_config` from the get go, to avoid having to run `git_config()` twice. I am not sure I got it right, but if I did: Running `git_config` with `git_diff_basic_config` from the beginning wouldn't be pointless when we would use any other command than `create`, `push` and `save`? Although it might confuse the reader a little bit, the stash should run without problems. Best, Paul
Re: [PATCH] fetch-pack: approximate no_dependents with filter
Jonathan Tan writes: > Whenever a lazy fetch is performed for a tree object, any trees and > blobs it directly or indirectly references will be fetched as well. > There is a "no_dependents" argument in struct fetch_pack_args that > indicates that objects that the wanted object references need not be > sent, but it currently has no effect other than to inhibit usage of > object flags. > > Extend the "no_dependents" argument to also exclude sending of objects > as much as the current protocol allows: when fetching a tree, all trees > it references will be sent (but not the blobs), and when fetching a > blob, it will still be sent. (If this mechanism is used to fetch a > commit or any other non-blob object, all referenced objects, except > blobs, will be sent.) The client neither needs to know or specify the > type of each object it wants. > > The necessary code change is done in fetch_pack() instead of somewhere > closer to where the "filter" instruction is written to the wire so that > only one part of the code needs to be changed in order for users of all > protocol versions to benefit from this optimization. It is very clear how you are churning the code, but it is utterly unclear from the description what you perceived as a problem and why this change is a good (if not the best) solution for that problem, at least to me. After reading the above description, I cannot shake the feeling that this is tied too strongly to the tree:0 use case? Does it help other use cases (e.g. would it be useful or harmful if a lazy clone was done to exclude blobs that are larger than certain threshold, or objects of all types that are not referenced by commits younger than certain threshold)?
Re: [PATCH 1/1] config doc: highlight the name=value syntax
Philip Oakley writes: > +Variable name/value syntax > +^^ > + > All the other lines (and the remainder of the line after the section > header) are recognized as setting variables, in the form > 'name = value' (or just 'name', which is a short-hand to say that > @@ -69,7 +72,8 @@ stripped. Leading whitespaces after 'name =', the > remainder of the > line after the first comment character '#' or ';', and trailing > whitespaces of the line are discarded unless they are enclosed in > double quotes. Internal whitespaces within the value are retained > -verbatim. > +verbatim. Single quotes are not special and form part of the > +variable's value. > > Inside double quotes, double quote `"` and backslash `\` characters > must be escaped: use `\"` for `"` and `\\` for `\`. Hmph. This feels a bit backwards. The original paragraph is horrible in that there is no clear mention that a pair of dq can be used to quote (which primarily is useful if your value have leading or trailing whitespaces); the closest hint is "enclosed in double quotes" we see in the pre-context. The added sentence singles out sq but it is unclear why it is necessary to call out that it is not special---the readers can legitimately wonder if backquotes are special or not and why. I wonder if this is easier to understand: diff --git a/Documentation/config.txt b/Documentation/config.txt index ad0f4510c3..5eebd539df 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -61,12 +61,16 @@ the variable is the boolean "true"). The variable names are case-insensitive, allow only alphanumeric characters and `-`, and must start with an alphabetic character. +The value part can have segments that are enclosed in a pair of +double quotes (note: other kinds of quoting character pairs are not +special)--the double quotes are stripped from the value. + A line that defines a value can be continued to the next line by ending it with a `\`; the backquote and the end-of-line are stripped. Leading whitespaces after 'name =', the remainder of the line after the first comment character '#' or ';', and trailing -whitespaces of the line are discarded unless they are enclosed in -double quotes. Internal whitespaces within the value are retained +whitespaces of the line are discarded. +Internal whitespaces within the value are retained verbatim. Inside double quotes, double quote `"` and backslash `\` characters > @@ -89,10 +93,14 @@ each other with the exception that `includeIf` sections > may be ignored > if their condition does not evaluate to true; see "Conditional includes" > below. > > +Both the `include` and `includeIf` sections implicitly apply an 'if found' > +condition to the given path names. > + Mentioning that missing target file is not an error is definitely an improvement. I've never viewed it as applying "if found" condition myself, but it is not wrong per-se to do so, I would think. > You can include a config file from another by setting the special > `include.path` (or `includeIf.*.path`) variable to the name of the file > to be included. The variable takes a pathname as its value, and is > -subject to tilde expansion. These variables can be given multiple times. > +subject to tilde expansion and the value syntax detailed above. > +These variables can be given multiple times. I have a mild suspicion that this adds negative value. Singling out that "[include] path = ..." follows the usual value syntax makes the readers wonder if there are some "[section] variable = ..." that does not follow the value syntax that they have to be aware of and careful about.
Urgent,
Hello i have been trying to contact you i have a transaction for you.
Re: [PATCH v3 1/5] CodingGuidelines: add shell piping guidelines
On Mon, Sep 24, 2018 at 2:03 PM SZEDER Gábor wrote: > > + - In a piped chain such as "grep blob objects | sort", the exit codes > > Let's make an example with git in it, e.g. something like this: > > git cmd | grep important | sort > > since just two lines below the new text mentions git crashing. Done. > > + - The $(git ...) construct also discards git's exit code, so if the > > This contruct is called command substitution, and it does preserve the > command's exit code, when the expanded text is assigned to a variable: > > $ var=$(exit 42) ; echo $? > 42 > > Note, however, that even in that case only the exit code of the last > command substitution is preserved: > > $ var=$(exit 1)foo$(exit 2)bar$(exit 3) ; echo $? > 3 > OK, I've changed this guideline to allow for setting a variable with command substitution, but not in other contexts. It's worded sufficiently openly such that your latter example will be forbidden. > > + goal is to test that particular command, redirect its output to a > > + temporary file rather than wrap it with $( ). > > I find this a bit vague, and to me it implies that ignoring the exit > code of a git command that is not the main focus of the given test is > acceptable, e.g. (made up pseudo example): > > test_expect_success 'fetch gets what it should' ' > git fetch $remote && > test "$(git rev-parse just-fetched)" = $expected_oid > ' > > In my opinion no tests should ignore the exit code of any git > command, ever. This seems like a pretty strong assertion, but something very similar is written in t/README (in the "don't" section): - use '! git cmd' when you want to make sure the git command exits with failure in a controlled way by calling "die()". Instead, use 'test_must_fail git cmd'. This will signal a failure if git dies in an unexpected way (e.g. segfault). So I've changed this to basically say you should never ignore git's exit code. Here is the new commit with updated message (I will wait for a day or two before I send a reroll): Documentation: add shell guidelines Add the following guideline to Documentation/CodingGuidelines: &&, ||, and | should appear at the end of lines, not the beginning, and the \ line continuation character should be omitted And the following to t/README (since it is specific to writing tests): pipes and $(git ...) should be avoided when they swallow exit codes of Git processes Signed-off-by: Matthew DeVore diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index 48aa4edfb..3d2cfea9b 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -118,6 +118,24 @@ For shell scripts specifically (not exhaustive): do this fi + - If a command sequence joined with && or || or | spans multiple + lines, put each command on a separate line and put && and || and | + operators at the end of each line, rather than the start. This + means you don't need to use \ to join lines, since the above + operators imply the sequence isn't finished. + +(incorrect) +grep blob verify_pack_result \ +| awk -f print_1.awk \ +| sort >actual && +... + +(correct) +grep blob verify_pack_result | +awk -f print_1.awk | +sort >actual && +... + - We prefer "test" over "[ ... ]". - We do not write the noiseword "function" in front of shell @@ -163,7 +181,6 @@ For shell scripts specifically (not exhaustive): does not have such a problem. - For C programs: - We use tabs to indent, and interpret tabs as taking up to diff --git a/t/README b/t/README index 9028b47d9..3e28b72c4 100644 --- a/t/README +++ b/t/README @@ -461,6 +461,32 @@ Don't: platform commands; just use '! cmd'. We are not in the business of verifying that the world given to us sanely works. + - Use Git upstream in the non-final position in a piped chain, as in: + + git -C repo ls-files | + xargs -n 1 basename | + grep foo + + which will discard git's exit code and may mask a crash. In the + above example, all exit codes are ignored except grep's. + + Instead, write the output of that command to a temporary + file with ">" or assign it to a variable with "x=$(git ...)" rather + than pipe it. + + - Use command substitution in a way that discards git's exit code. + When assigning to a variable, the exit code is not discarded, e.g.: + + x=$(git cat-file -p $sha) && + ... + + is OK because a crash in "git cat-file" will cause the "&&" chain + to fail, but: + + test_cmp expect $(git cat-file -p $sha) + + is not OK and a crash in git could go undetected. + - use perl without spelling it as "$PERL_PATH". This is to help our friends on Windows where the platform Perl often adds CR before the end of line, and they bundle Git with a version of Perl that > > > These last
Urgent,
Hello i have been trying to contact you i have a transaction for you.
[ANNOUNCE] New Git PLC Members
I mentioned about a month ago in [1] that we needed to add members to the committee representing the Git project as part of Software Freedom Conservancy (see that email for details on what exactly that means :) ). I'm happy to announce that this is now done, and both Christian and Ævar are on the committee. Welcome to both of you! As before, if you have project governance questions, the best point of contact is g...@sfconservancy.org, which goes to the four committee members, along with a few Conservancy folks. Though unless it specifically needs to be private, I'd generally encourage people to raise issues first on the list so the whole community can discuss. -Peff [1] https://public-inbox.org/git/20180816224138.ga15...@sigill.intra.peff.net/
Urgent,
Hello i have been trying to contact you i have a transaction for you.
Re: git fetch behaves weirdely when run in a worktree
Kaartic Sivaraam writes: >> Also please try >> "git fetch" again with GIT_TRACE=1 and GIT_TRACE_SETUP=1. Hopefully we >> could catch something with that. > > $ GIT_TRACE_SETUP=1 GIT_TRACE=1 git fetch origin next > 23:10:26.049785 trace.c:377 setup: git_dir: > $COMMON_ROOT/git/.git/worktrees/git-next-build-automate > ... > 23:10:28.402815 git.c:415 trace: built-in: git rev-list > --objects --stdin --not --all --quiet > From https://github.com/git/git > * branch next -> FETCH_HEAD > 23:10:28.437350 run-command.c:1553 run_processes_parallel: preparing to > run up to 1 tasks > ... That looks like fetching only the 'next' branch and nothing else to me. Perhaps your script is referring to a variable whose assignment is misspelled and invoking git fetch $origin $branch and you are expecting the $branch variable to have string 'next' but due to some bugs it is empty somehow? That explains why sometimes (i.e. when $branch does not get the value you expect it to have) the script fetches everything and some other times (i.e. when $branch does get the right value) the script fetches only the named branch.
Re: [PATCH] worktree: add per-worktree config files
Nguyễn Thái Ngọc Duy writes: > This extension is most useful in multiple worktree setup because you > now have an option to store per-worktree config (which is either > .git/config.worktree for main worktree, or > .git/worktrees/xx/config.worktree for linked ones). Heh. "This is useful if you have multiple" is true without saying, because this is totally useless if you have only a single worktree. I'd suggest writing the above without "most useful". > This design places a bet on the assumption that the majority of config > variables are shared so it is the default mode. A safer move would be > default writes go to per-worktree file, so that accidental changes are > isolated. Warning: devil's advocate mode on. Done in either way, this will confuse the users. What is the reason why people are led to think it is a good idea to use multiple worktrees, even when they need different settings? What do they want out of "multiple worktrees linked to a single repository" as opposed to just a simple "as many clones as necessary"? Reduced disk footprint? Is there a better way to achieve that without the downside of multiple worktrees (e.g. configuration need to be uniform)? > (*) "git config --worktree" points back to "config" file when this > extension is not present so that it works in any setup. Shouldn't it barf and error out instead? A user who hasn't enabled the extension uses --worktree option and misled to believe that the setting affects only a single worktree, even though the change is made globally---that does not sound like a great end-user experience.
Re: [PATCH 1/1] read-cache: update index format default to v4
On 9/25/2018 2:01 PM, Stefan Beller wrote: On Tue, Sep 25, 2018 at 7:30 AM Derrick Stolee wrote: On 9/25/2018 3:06 AM, Patrick Steinhardt wrote: On Mon, Sep 24, 2018 at 11:32:23PM +0200, SZEDER Gábor wrote: On Mon, Sep 24, 2018 at 02:15:30PM -0700, Derrick Stolee via GitGitGadget wrote: From: Derrick Stolee The index v4 format has been available since 2012 with 9d22778 "reach-cache.c: write prefix-compressed names in the index". Since the format has been stable for so long, almost all versions of Git in use today understand version 4, removing one barrier to upgrade -- that someone may want to downgrade and needs a working repo. What about alternative implementations, like JGit, libgit2, etc.? Speaking of libgit2, we are able to read and write index v4 since commit c1b370e93 This is a good point, Szeder. Patrick: I'm glad LibGit2 is up-to-date with index formats. Unfortunately, taking a look (for the first time) at the JGit code reveals that they don't appear to have v4 support. In org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCache.java, the DirCache.readFrom() method: lines 488-494, I see the following snippet: final int ver = NB.decodeInt32(hdr, 4); boolean extended = false; if (ver == 3) extended = true; else if (ver != 2) throw new CorruptObjectException(MessageFormat.format( JGitText.get().unknownDIRCVersion, Integer.valueOf(ver))); It looks like this will immediately throw with versions other than 2 or 3. I'm adding Jonathan Nieder to CC so he can check with JGit people about the impact of this change. JGit is used both on the server (which doesn't use index/staging area) as well as client side as e.g. an Eclipse integration, which would very much like to use the index. Adding Matthias Sohn as well, who is active in JGit and cares more about the client side than Googlers who only make use of the server side part of JGit. Stefan In addition to the compatibility with other implementations of git question, I think we should include Duy's patch [1] to "optimize reading index format v4" before flipping the default to V4. In my testing, this reduced the index load time of V4 by 10%-15% making the CPU cost only ~2% more expensive than V2 or V3 indexes. This increase in CPU cost is more than offset by the significant reduction in file IO due to the reduced index file size. [1] https://public-inbox.org/git/20180825064458.28484-1-pclo...@gmail.com/
Re: [PATCH 3/8] refs: new ref types to make per-worktree refs visible to all worktrees
Nguyễn Thái Ngọc Duy writes:
> The main worktree has to be treated specially because well.. it's
> special from the beginning. So HEAD from the main worktree is
> acccessible via the name "main/HEAD" (we can't use
> "worktrees/main/HEAD" because "main" under "worktrees" is not
> reserved).
I do not quite follow. So with this, both refs/heads/master and
main/refs/heads/master are good names for the master branch (even
though the local branch names are not per worktree), because
in the main worktree, refs/bisect/bad and main/refs/bisect/bad ought
to mean the same thing.
side note: Or is this only for pseudo-refs
(i.e. $GIT_DIR/$name where $name consists of all caps or
underscore and typically ends with HEAD)? Even if that were
the case, I do not think it essentially changes the issue
around disambiguation that much.
The disambiguation rule has always been: if you have a confusingly
named ref, you can spell it out fully to avoid any ambiguity, e.g.
refs/heads/refs/heads/foo can be given to "git rev-parse" and will
mean the tip of the branch whose name is "refs/heads/foo", even when
another branch whose name is "foo" exists.
Would we have a reasonable disambiguation rules that work well with
the main/ and worktrees/* prefixes? When somebody has main/HEAD branch
and writes "git rev-parse main/HEAD", does it find refs/heads/main/HEAD
or $GIT_DIR/HEAD, if the user is in the main worktree?
It could be simply that the design is underdocumented in this patch
set (in which case I would have appreciated 'RFC' near 'PATCH'), but
I have a feeling that the code came way too early before such design
issues are fleshed out.
> diff --git a/refs.h b/refs.h
> index bd52c1bbae..9b53dbeae8 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -704,9 +704,11 @@ int parse_hide_refs_config(const char *var, const char
> *value, const char *);
> int ref_is_hidden(const char *, const char *);
>
> enum ref_type {
> - REF_TYPE_PER_WORKTREE,
> - REF_TYPE_PSEUDOREF,
> - REF_TYPE_NORMAL,
> + REF_TYPE_PER_WORKTREE,/* refs inside refs/ but not shared */
> + REF_TYPE_PSEUDOREF, /* refs outside refs/ in current worktree */
> + REF_TYPE_MAIN_PSEUDOREF, /* pseudo refs from the main worktree */
> + REF_TYPE_OTHER_PSEUDOREF, /* pseudo refs from other worktrees */
> + REF_TYPE_NORMAL, /* normal/shared refs inside refs/*/
> };
>
> enum ref_type ref_type(const char *refname);
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 416eafa453..bf9ed633b1 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -149,6 +149,23 @@ static struct files_ref_store *files_downcast(struct
> ref_store *ref_store,
> return refs;
> }
>
> +static void files_reflog_path_other_worktrees(struct files_ref_store *refs,
> + struct strbuf *sb,
> + const char *refname)
> +{
> + const char *real_ref;
> +
> + if (!skip_prefix(refname, "worktrees/", _ref))
> + BUG("refname %s is not a other-worktree ref", refname);
> + real_ref = strchr(real_ref, '/');
> + if (!real_ref)
> + BUG("refname %s is not a other-worktree ref", refname);
> + real_ref++;
> +
> + strbuf_addf(sb, "%s/%.*slogs/%s", refs->gitcommondir,
> + (int)(real_ref - refname), refname, real_ref);
> +}
> +
> static void files_reflog_path(struct files_ref_store *refs,
> struct strbuf *sb,
> const char *refname)
> @@ -158,6 +175,12 @@ static void files_reflog_path(struct files_ref_store
> *refs,
> case REF_TYPE_PSEUDOREF:
> strbuf_addf(sb, "%s/logs/%s", refs->gitdir, refname);
> break;
> + case REF_TYPE_OTHER_PSEUDOREF:
> + return files_reflog_path_other_worktrees(refs, sb, refname);
> + case REF_TYPE_MAIN_PSEUDOREF:
> + if (!skip_prefix(refname, "main/", ))
> + BUG("ref %s is not a main pseudoref", refname);
> + /* passthru */
> case REF_TYPE_NORMAL:
> strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname);
> break;
> @@ -176,6 +199,11 @@ static void files_ref_path(struct files_ref_store *refs,
> case REF_TYPE_PSEUDOREF:
> strbuf_addf(sb, "%s/%s", refs->gitdir, refname);
> break;
> + case REF_TYPE_MAIN_PSEUDOREF:
> + if (!skip_prefix(refname, "main/", ))
> + BUG("ref %s is not a main pseudoref", refname);
> + /* passthru */
> + case REF_TYPE_OTHER_PSEUDOREF:
> case REF_TYPE_NORMAL:
> strbuf_addf(sb, "%s/%s", refs->gitcommondir, refname);
> break;
> diff --git a/t/t1415-worktree-refs.sh b/t/t1415-worktree-refs.sh
> index 0c2d5f89a9..46ca7bfc19 100755
> --- a/t/t1415-worktree-refs.sh
> +++
Re: Git Test Coverage Report (Tuesday, Sept 25)
On 9/25/2018 2:42 PM, Derrick Stolee wrote:
In an effort to ensure new code is reasonably covered by the test suite,
we now have contrib/coverage-diff.sh to combine the gcov output from
'make coverage-test ; make coverage-report' with the output from 'git
diff A B' to discover _new_ lines of code that are not covered.
This report takes the output of these results after running on four
branches:
pu: 80e728fa913dc3a1165b6dec9a7afa6052a86325
jch: 0c10634844314ab89666ed0a1c7d36dde7ac9689
next: 76f2f5c1e34c4dbef1029e2984c2892894c444ce
master: fe8321ec057f9231c26c29b364721568e58040f7
master@{1}: 2d3b1c576c85b7f5db1f418907af00ab88e0c303
I ran the test suite on each of these branches on an Ubuntu Linux VM,
and I'm missing some dependencies (like apache, svn, and perforce) so
not all tests are run.
I submit this output without comment. I'm taking a look especially at my
own lines to see where coverage can be improved.
Thanks for driving this. I think it provides an interesting view into
new code and how well it is being tested. In an effort to make this as
useful as possible, we should be looking to eliminate as much noise as
possible otherwise people will stop looking at it.
I looked at the lines that came from my patches and most if not all of
them are only going to be executed by the test suite if the correct
"special setup" option is enabled. In my particular case, that is the
option "GIT_TEST_INDEX_THREADS=" as documented in t/README.
I suspect this will be the case for other code as well so I wonder if
the tests should be run with each the GIT_TEST_* options that exist to
exercise uncommon code paths with the test suite. This should prevent
false positives on code paths that are actually covered by the test
suite as long as it is run with the appropriate option set.
I realize it would take a long time to run the entire test suite with
all GIT_TEST_* variables so perhaps they can only be tested "as needed"
(ie when patches add new variables in the "special setups" section of
t/README). This should reduce the number of combinations that need to
be run while still eliminating many of the false positive hits.
Re: [PATCH] ref-filter: don't look for objects when outside of a repository
SZEDER Gábor writes: > However, if we go for a more informative error message, then wouldn't > it be better to add this condition in populate_value() before it even > calls get_object()? Then we could also add the problematic format > specifier to the error message (I think, but didn't actually check), > just in case someone specified multiple sort keys. Even though I suspect that verify_ref_format() is the logically the right place to do this (after all, it is about seeing if the format makes sense, and a format that requires an object access used outside a repository should trigger an verification error), doing that in populate_value() probably strikes the best balance, I would think.
Re: [PATCH] git help: promote 'git help -av'
On Tue, Sep 25, 2018 at 07:44:51PM +0200, Duy Nguyen wrote:
> > I think adding another section about external commands in "help -av"
> > would address the "clang-format" stuff. With that, it's probably good
> > enough to completely replace "help -a". It may also be good to list
> > aliases there too in a separate section so you have "all you can type"
> > in one (big) list.
>
> Here's the patch that adds that external commands and aliases
> sections. I feel that external commands section is definitely good to
> have even if we don't replace "help -a". Aliases are more
> subjective...
Just eyeballing the output, it looks pretty reasonable to me. The lack
of explanation for external commands really stands out, but there's not
much we can do.
That part of the output is not very compact, and we _could_ put it in
columns, but that might be confusing since the rest of the output is
one-per-line.
Mentioning aliases seems reasonable to me. The definitions of some of
mine are pretty nasty bits of shell, but I guess that people either
don't have any ugly aliases, or are comfortable enough with them that
they won't be scared away. :)
> -- 8< --
> @@ -53,7 +52,6 @@ static struct option builtin_help_options[] = {
> HELP_FORMAT_WEB),
> OPT_SET_INT('i', "info", _format, N_("show info page"),
> HELP_FORMAT_INFO),
> - OPT__VERBOSE(, N_("print command description")),
> OPT_END(),
> };
Would we want to continue respecting "-v" as a noop? I admit I did not
even know it existed until this thread, but if people have trained
themselves to run "git help -av", we should probably continue to give
them this output.
-Peff
Re: [PATCH 1/3] t7001: reformat to newer style
Stefan Beller writes:
> Heh, thanks for calling that out. So we're looking at a full formatter
> instead of a partial formatter that helps moving in the right direction now.
> :-/
The parts left out of these patches (e.g. use subshell when working
in a subdirectory) need human decision and a bit of good taste, and
I think it was a good design decision to keep these three patches
all mechanical. There do need a follow-up [PATCH {4,5,6}/3] that
cannot be done mechanically, though, before the result can be called
"this script has been cleaned up and new people are encouraged to
mimick the way it does things".
That way, we can keep the mechanically good bits that are early in
the series while polishing the later bits that need human judgement.
Greeting
Hello Dear, I am working in financial firm in Asia. I have a business to transfer i have abandon $19.000.000.00 in my office. If you are interested in the transaction, please reply for full information. Best Regards, Financial Asia
Re: [PATCH] commit-reach: cleanups in can_all_from_reach...
Derrick Stolee writes: > Another commit walk that could be improved by generation numbers? It's > like my bat-signal! Ah, nevermind. The "traversal" done by these helper functions is the most stupid kind (not the algorthim, but the need). It's not like there is an opportunity to optimize by not traversing the remainder if we choose the order of traversal intelligently, so any algorithm that does not visit the same node twice and does not do the most naive recursion would work, and what we currently have there is just fine.
Re: Git for games working group
On Mon, Sep 24, 2018 at 09:05:56PM -0700, John Austin wrote: > On Mon, Sep 24, 2018 at 12:58 PM Taylor Blau wrote: > > I'm replying to this part of the email to note that this would cause Git > > LFS to have to do some extra work, since running 'git lfs install' > > already writes to .git/hooks/post-commit (ironically, to detect and > > unlock locks that we should have released). > > Right, that should have been another bullet point. The fact that there > can only be one git hook is.. frustrating. Sure, I think one approach to dealing with this is to teach Git how to handle multiple hooks for the same phase of hook. I don't know how likely this is in practice to be something that would be acceptable, since it seems to involve much more work than either of our tools learning about the other. > Perhaps, if LFS has an option to bundle global-graph, LFS could merge > the hooks when installing? Right. I think that (in an ideal world) both tools would know about the other, that way we can not have to worry about who installs what first. > If you instead install global-graph after LFS, I think it should > probably attempt something like: > -- first move the existing hook to a folder: post-commit.d/ > -- install the global-graph hook to post-commit.d/ > -- install a new hook at post-commit that simply calls all > executables in post-commit.d/ > > Not sure if this is something that's been discussed, since I know LFS > has a similar issue with existing hooks, but might be sensible. Yeah, I think that that would be fine, too. Thanks, Taylor
[PATCH v4 4/9] submodule: move global changed_submodule_names into fetch submodule struct
The `changed_submodule_names` are only used for fetching, so let's make it
part of the struct that is passed around for fetching submodules.
Signed-off-by: Stefan Beller
---
submodule.c | 42 +++---
1 file changed, 23 insertions(+), 19 deletions(-)
diff --git a/submodule.c b/submodule.c
index 22c64bd8559..17103379ba4 100644
--- a/submodule.c
+++ b/submodule.c
@@ -25,7 +25,7 @@
#include "commit-reach.h"
static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
-static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
+
static int initialized_fetch_ref_tips;
static struct oid_array ref_tips_before_fetch;
static struct oid_array ref_tips_after_fetch;
@@ -1110,7 +1110,22 @@ void check_for_new_submodule_commits(struct object_id
*oid)
oid_array_append(_tips_after_fetch, oid);
}
-static void calculate_changed_submodule_paths(void)
+struct submodule_parallel_fetch {
+ int count;
+ struct argv_array args;
+ struct repository *r;
+ const char *prefix;
+ int command_line_option;
+ int default_option;
+ int quiet;
+ int result;
+
+ struct string_list changed_submodule_names;
+};
+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0,
STRING_LIST_INIT_DUP }
+
+static void calculate_changed_submodule_paths(
+ struct submodule_parallel_fetch *spf)
{
struct argv_array argv = ARGV_ARRAY_INIT;
struct string_list changed_submodules = STRING_LIST_INIT_DUP;
@@ -1148,7 +1163,8 @@ static void calculate_changed_submodule_paths(void)
continue;
if (!submodule_has_commits(path, commits))
- string_list_append(_submodule_names,
name->string);
+ string_list_append(>changed_submodule_names,
+ name->string);
}
free_submodules_oids(_submodules);
@@ -1185,18 +1201,6 @@ int submodule_touches_in_range(struct object_id
*excl_oid,
return ret;
}
-struct submodule_parallel_fetch {
- int count;
- struct argv_array args;
- struct repository *r;
- const char *prefix;
- int command_line_option;
- int default_option;
- int quiet;
- int result;
-};
-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}
-
static int get_fetch_recurse_config(const struct submodule *submodule,
struct submodule_parallel_fetch *spf)
{
@@ -1257,7 +1261,7 @@ static int get_next_submodule(struct child_process *cp,
case RECURSE_SUBMODULES_ON_DEMAND:
if (!submodule ||
!string_list_lookup(
- _submodule_names,
+ >changed_submodule_names,
submodule->name))
continue;
default_argv = "on-demand";
@@ -1349,8 +1353,8 @@ int fetch_populated_submodules(struct repository *r,
argv_array_push(, "--recurse-submodules-default");
/* default value, "--submodule-prefix" and its value are added later */
- calculate_changed_submodule_paths();
- string_list_sort(_submodule_names);
+ calculate_changed_submodule_paths();
+ string_list_sort(_submodule_names);
run_processes_parallel(max_parallel_jobs,
get_next_submodule,
fetch_start_failure,
@@ -1359,7 +1363,7 @@ int fetch_populated_submodules(struct repository *r,
argv_array_clear();
out:
- string_list_clear(_submodule_names, 1);
+ string_list_clear(_submodule_names, 1);
return spf.result;
}
--
2.19.0.605.g01d371f741-goog
[PATCH v4 3/9] submodule.c: sort changed_submodule_names before searching it
We can string_list_insert() to maintain sorted-ness of the list as we find new items, or we can string_list_append() to build an unsorted list and sort it at the end just once. To pick which one is more appropriate, we notice the fact that we discover new items more or less in the already sorted order. That makes "append then sort" more appropriate. Signed-off-by: Stefan Beller --- submodule.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/submodule.c b/submodule.c index 0de9e2800ad..22c64bd8559 100644 --- a/submodule.c +++ b/submodule.c @@ -1256,7 +1256,7 @@ static int get_next_submodule(struct child_process *cp, case RECURSE_SUBMODULES_DEFAULT: case RECURSE_SUBMODULES_ON_DEMAND: if (!submodule || - !unsorted_string_list_lookup( + !string_list_lookup( _submodule_names, submodule->name)) continue; @@ -1350,6 +1350,7 @@ int fetch_populated_submodules(struct repository *r, /* default value, "--submodule-prefix" and its value are added later */ calculate_changed_submodule_paths(); + string_list_sort(_submodule_names); run_processes_parallel(max_parallel_jobs, get_next_submodule, fetch_start_failure, -- 2.19.0.605.g01d371f741-goog
[PATCH v4 1/9] sha1-array: provide oid_array_filter
Helped-by: Junio C Hamano
Signed-off-by: Stefan Beller
---
Documentation/technical/api-oid-array.txt | 5 +
sha1-array.c | 17 +
sha1-array.h | 3 +++
3 files changed, 25 insertions(+)
diff --git a/Documentation/technical/api-oid-array.txt
b/Documentation/technical/api-oid-array.txt
index 9febfb1d528..c97428c2c34 100644
--- a/Documentation/technical/api-oid-array.txt
+++ b/Documentation/technical/api-oid-array.txt
@@ -48,6 +48,11 @@ Functions
is not sorted, this function has the side effect of sorting
it.
+`oid_array_filter`::
+ Apply the callback function `want` to each entry in the array,
+ retaining only the entries for which the function returns true.
+ Preserve the order of the entries that are retained.
+
Examples
diff --git a/sha1-array.c b/sha1-array.c
index b94e0ec0f5e..d922e94e3fc 100644
--- a/sha1-array.c
+++ b/sha1-array.c
@@ -77,3 +77,20 @@ int oid_array_for_each_unique(struct oid_array *array,
}
return 0;
}
+
+void oid_array_filter(struct oid_array *array,
+ for_each_oid_fn want,
+ void *cb_data)
+{
+ unsigned nr = array->nr, src, dst;
+ struct object_id *oids = array->oid;
+
+ for (src = dst = 0; src < nr; src++) {
+ if (want([src], cb_data)) {
+ if (src != dst)
+ oidcpy([dst], [src]);
+ dst++;
+ }
+ }
+ array->nr = dst;
+}
diff --git a/sha1-array.h b/sha1-array.h
index 232bf950172..55d016c4bf7 100644
--- a/sha1-array.h
+++ b/sha1-array.h
@@ -22,5 +22,8 @@ int oid_array_for_each(struct oid_array *array,
int oid_array_for_each_unique(struct oid_array *array,
for_each_oid_fn fn,
void *data);
+void oid_array_filter(struct oid_array *array,
+ for_each_oid_fn want,
+ void *cbdata);
#endif /* SHA1_ARRAY_H */
--
2.19.0.605.g01d371f741-goog
[PATCH v4 2/9] submodule.c: fix indentation
The submodule subsystem is really bad at staying within 80 characters.
Fix it while we are here.
Signed-off-by: Stefan Beller
---
submodule.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/submodule.c b/submodule.c
index b53cb6e9c47..0de9e2800ad 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1244,7 +1244,8 @@ static int get_next_submodule(struct child_process *cp,
if (!submodule) {
const char *name = default_name_or_path(ce->name);
if (name) {
- default_submodule.path = default_submodule.name
= name;
+ default_submodule.path = name;
+ default_submodule.name = name;
submodule = _submodule;
}
}
@@ -1254,8 +1255,10 @@ static int get_next_submodule(struct child_process *cp,
default:
case RECURSE_SUBMODULES_DEFAULT:
case RECURSE_SUBMODULES_ON_DEMAND:
- if (!submodule ||
!unsorted_string_list_lookup(_submodule_names,
-submodule->name))
+ if (!submodule ||
+ !unsorted_string_list_lookup(
+ _submodule_names,
+ submodule->name))
continue;
default_argv = "on-demand";
break;
--
2.19.0.605.g01d371f741-goog
[PATCH v4 5/9] submodule.c: do not copy around submodule list
'calculate_changed_submodule_paths' uses a local list to compute the
changed submodules, and then produces the result by copying appropriate
items into the result list.
Instead use the result list directly and prune items afterwards
using string_list_remove_empty_items.
By doing so we'll have access to the util pointer for longer that
contains the commits that we need to fetch, which will be
useful in a later patch.
Signed-off-by: Stefan Beller
---
submodule.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/submodule.c b/submodule.c
index 17103379ba4..dd478ed70bf 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1128,8 +1128,7 @@ static void calculate_changed_submodule_paths(
struct submodule_parallel_fetch *spf)
{
struct argv_array argv = ARGV_ARRAY_INIT;
- struct string_list changed_submodules = STRING_LIST_INIT_DUP;
- const struct string_list_item *name;
+ struct string_list_item *name;
/* No need to check if there are no submodules configured */
if (!submodule_from_path(the_repository, NULL, NULL))
@@ -1146,9 +1145,9 @@ static void calculate_changed_submodule_paths(
* Collect all submodules (whether checked out or not) for which new
* commits have been recorded upstream in "changed_submodule_names".
*/
- collect_changed_submodules(_submodules, );
+ collect_changed_submodules(>changed_submodule_names, );
- for_each_string_list_item(name, _submodules) {
+ for_each_string_list_item(name, >changed_submodule_names) {
struct oid_array *commits = name->util;
const struct submodule *submodule;
const char *path = NULL;
@@ -1162,12 +1161,14 @@ static void calculate_changed_submodule_paths(
if (!path)
continue;
- if (!submodule_has_commits(path, commits))
- string_list_append(>changed_submodule_names,
- name->string);
+ if (submodule_has_commits(path, commits)) {
+ oid_array_clear(commits);
+ *name->string = '\0';
+ }
}
- free_submodules_oids(_submodules);
+ string_list_remove_empty_items(>changed_submodule_names, 1);
+
argv_array_clear();
oid_array_clear(_tips_before_fetch);
oid_array_clear(_tips_after_fetch);
@@ -1363,7 +1364,7 @@ int fetch_populated_submodules(struct repository *r,
argv_array_clear();
out:
- string_list_clear(_submodule_names, 1);
+ free_submodules_oids(_submodule_names);
return spf.result;
}
--
2.19.0.605.g01d371f741-goog
[PATCH v4 0/9] fetch: make sure submodule oids are fetched
v4:
* Per Ævars comment, moved the docs for oid_array_filter into
Documentation/technical/...
* addressed all outstanding comment as noted in "What's cooking" email,
* below is a range-diff against the currently queued version of the
series.
v3:
* I discovered some issues with v2 after sending,
which is why I rewrote the later patches completely
and now we pass around a "task" struct that contains everything to know
about the things to work on and what needs free()ing afterwards.
* as it is no longer string list based, this drops adding string_list_{pop,
last}
v2:
* extended commit messages,
* plugged a memory leak
* rewrote the patch "sha1-array: provide oid_array_filter" to be much more like
object_array_fiter
* fixed a typo pointed out by Ramsay.
The range diff is below.
Thanks,
Stefan
v1:
Currently when git-fetch is asked to recurse into submodules, it dispatches
a plain "git-fetch -C " (and some submodule related options
such as prefix and recusing strategy, but) without any information of the
remote or the tip that should be fetched.
This works surprisingly well in some workflows, not so well in others,
which this series aims to fix.
The first patches provide new basic functionality and do some refactoring;
the interesting part is in the two last patches.
This was discussed in
https://public-inbox.org/git/20180808221752.195419-1-sbel...@google.com/
and I think I addressed all feedback so far.
Stefan Beller (9):
sha1-array: provide oid_array_filter
submodule.c: fix indentation
submodule.c: sort changed_submodule_names before searching it
submodule: move global changed_submodule_names into fetch submodule
struct
submodule.c: do not copy around submodule list
repository: repo_submodule_init to take a submodule struct
submodule: fetch in submodules git directory instead of in worktree
fetch: retry fetching submodules if needed objects were not fetched
builtin/fetch: check for submodule updates for non branch fetches
Documentation/technical/api-oid-array.txt | 5 +
builtin/fetch.c | 14 +-
builtin/grep.c| 17 +-
builtin/ls-files.c| 12 +-
builtin/submodule--helper.c | 2 +-
repository.c | 27 +--
repository.h | 11 +-
sha1-array.c | 17 ++
sha1-array.h | 3 +
submodule.c | 275 +-
t/t5526-fetch-submodules.sh | 23 +-
11 files changed, 311 insertions(+), 95 deletions(-)
git range-diff origin/sb/submodule-recursive-fetch-gets-the-tip...
1: 6fecf7cd01a < -: --- string-list: add string_list_{pop, last}
functions
2: 7007a318a68 < -: --- sha1-array: provide oid_array_filter
[ ... snip ... I rebased onto: ]
-: --- > 215: fe8321ec057 Second batch post 2.19
-: --- > 216: a9b49d4cfe9 sha1-array: provide oid_array_filter
3: 807429234ac ! 217: 813205700d1 submodule.c: fix indentation
@@ -6,7 +6,6 @@
Fix it while we are here.
Signed-off-by: Stefan Beller
-Signed-off-by: Junio C Hamano
diff --git a/submodule.c b/submodule.c
--- a/submodule.c
4: f6fa5273af9 ! 218: b4aa77f72ba submodule.c: sort changed_submodule_names
before searching it
@@ -12,7 +12,6 @@
appropriate.
Signed-off-by: Stefan Beller
-Signed-off-by: Junio C Hamano
diff --git a/submodule.c b/submodule.c
--- a/submodule.c
5: adf7a2fd203 ! 219: df4da0e18e4 submodule: move global
changed_submodule_names into fetch submodule struct
@@ -6,13 +6,12 @@
part of the struct that is passed around for fetching submodules.
Signed-off-by: Stefan Beller
-Signed-off-by: Junio C Hamano
diff --git a/submodule.c b/submodule.c
--- a/submodule.c
+++ b/submodule.c
@@
- #include "object-store.h"
+ #include "commit-reach.h"
static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
-static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
6: 56c9398589a ! 220: b9f5d06c134 submodule.c: do not copy around submodule
list
@@ -9,12 +9,11 @@
Instead use the result list directly and prune items afterwards
using string_list_remove_empty_items.
-By doin so we'll have access to the util pointer for longer that
+By doing so we'll have access to the util pointer for longer that
contains the commits that we need to fetch, which will be
useful in a later patch.
Signed-off-by: Stefan Beller
-Signed-off-by: Junio C Hamano
diff --git a/submodule.c b/submodule.c
--- a/submodule.c
-: --- > 221: e5ff287a0a2 repository: repo_submodule_init to take a
[PATCH v4 6/9] repository: repo_submodule_init to take a submodule struct
When constructing a struct repository for a submodule for some revision
of the superproject where the submodule is not contained in the index,
it may not be present in the working tree currently either. In that
siutation giving a 'path' argument is not useful. Upgrade the
repo_submodule_init function to take a struct submodule instead.
While we are at it, overhaul the repo_submodule_init function by renaming
the submodule repository struct, which is to be initialized, to a name
that is not confused with the struct submodule as easily.
Also move its documentation into the header file.
Signed-off-by: Stefan Beller
---
builtin/grep.c | 17 ++---
builtin/ls-files.c | 12 +++-
builtin/submodule--helper.c | 2 +-
repository.c| 27 ++-
repository.h| 11 +--
5 files changed, 37 insertions(+), 32 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 601f801158f..81c53c862b1 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -418,16 +418,19 @@ static int grep_submodule(struct grep_opt *opt, struct
repository *superproject,
const struct object_id *oid,
const char *filename, const char *path)
{
- struct repository submodule;
+ struct repository subrepo;
+ const struct submodule *sub = submodule_from_path(superproject,
+ _oid, path);
+
int hit;
if (!is_submodule_active(superproject, path))
return 0;
- if (repo_submodule_init(, superproject, path))
+ if (repo_submodule_init(, superproject, sub))
return 0;
- repo_read_gitmodules();
+ repo_read_gitmodules();
/*
* NEEDSWORK: This adds the submodule's object directory to the list of
@@ -440,7 +443,7 @@ static int grep_submodule(struct grep_opt *opt, struct
repository *superproject,
* object.
*/
grep_read_lock();
- add_to_alternates_memory(submodule.objects->objectdir);
+ add_to_alternates_memory(subrepo.objects->objectdir);
grep_read_unlock();
if (oid) {
@@ -465,14 +468,14 @@ static int grep_submodule(struct grep_opt *opt, struct
repository *superproject,
init_tree_desc(, data, size);
hit = grep_tree(opt, pathspec, , , base.len,
- object->type == OBJ_COMMIT, );
+ object->type == OBJ_COMMIT, );
strbuf_release();
free(data);
} else {
- hit = grep_cache(opt, , pathspec, 1);
+ hit = grep_cache(opt, , pathspec, 1);
}
- repo_clear();
+ repo_clear();
return hit;
}
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 7f9919a3623..4d1649c1b3a 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -206,17 +206,19 @@ static void show_files(struct repository *repo, struct
dir_struct *dir);
static void show_submodule(struct repository *superproject,
struct dir_struct *dir, const char *path)
{
- struct repository submodule;
+ struct repository subrepo;
+ const struct submodule *sub = submodule_from_path(superproject,
+ _oid, path);
- if (repo_submodule_init(, superproject, path))
+ if (repo_submodule_init(, superproject, sub))
return;
- if (repo_read_index() < 0)
+ if (repo_read_index() < 0)
die("index file corrupt");
- show_files(, dir);
+ show_files(, dir);
- repo_clear();
+ repo_clear();
}
static void show_ce(struct repository *repo, struct dir_struct *dir,
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 40844870cfb..1164e0979bd 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2037,7 +2037,7 @@ static int ensure_core_worktree(int argc, const char
**argv, const char *prefix)
if (!sub)
BUG("We could get the submodule handle before?");
- if (repo_submodule_init(, the_repository, path))
+ if (repo_submodule_init(, the_repository, sub))
die(_("could not get a repository handle for submodule '%s'"),
path);
if (!repo_config_get_string(, "core.worktree", )) {
diff --git a/repository.c b/repository.c
index 5dd14867181..aabe64ee5d9 100644
--- a/repository.c
+++ b/repository.c
@@ -166,30 +166,23 @@ int repo_init(struct repository *repo,
return -1;
}
-/*
- * Initialize 'submodule' as the submodule given by 'path' in parent repository
- * 'superproject'.
- * Return 0 upon success and a non-zero value upon failure.
- */
-int repo_submodule_init(struct repository *submodule,
+int repo_submodule_init(struct repository *subrepo,
struct repository
[PATCH v4 9/9] builtin/fetch: check for submodule updates for non branch fetches
Gerrit, the code review tool, has a different workflow than our mailing
list based approach. Usually users upload changes to a Gerrit server and
continuous integration and testing happens by bots. Sometimes however a
user wants to checkout a change locally and look at it locally. For this
use case, Gerrit offers a command line snippet to copy and paste to your
terminal, which looks like
git fetch https:///gerrit refs/changes/ &&
git checkout FETCH_HEAD
For Gerrit changes that contain changing submodule gitlinks, it would be
easy to extend both the fetch and checkout with the '--recurse-submodules'
flag, such that this command line snippet would produce the state of a
change locally.
However the functionality added in the previous patch, which would
ensure that we fetch the objects in the submodule that the gitlink pointed
at, only works for remote tracking branches so far, not for FETCH_HEAD.
Make sure that fetching a superproject to its FETCH_HEAD, also respects
the existence checks for objects in the submodule recursion.
Signed-off-by: Stefan Beller
---
builtin/fetch.c | 5 -
t/t5526-fetch-submodules.sh | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index e3b03ad3bd3..f2d9e548bf0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -894,11 +894,14 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
rc |= update_local_ref(ref, what, rm, ,
summary_width);
free(ref);
- } else
+ } else {
+ check_for_new_submodule_commits(>old_oid);
format_display(, '*',
*kind ? kind : "branch", NULL,
*what ? what : "HEAD",
"FETCH_HEAD", summary_width);
+ }
+
if (note.len) {
if (verbosity >= 0 && !shown_url) {
fprintf(stderr, _("From %.*s\n"),
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index af12c50e7dd..a509eabb044 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -615,7 +615,7 @@ test_expect_success "fetch new commits on-demand when they
are not reachable" '
git update-ref refs/changes/2 $D &&
(
cd downstream &&
- git fetch --recurse-submodules --recurse-submodules-default
on-demand origin refs/changes/2:refs/heads/my_branch &&
+ git fetch --recurse-submodules origin refs/changes/2 &&
git -C submodule cat-file -t $C &&
git checkout --recurse-submodules FETCH_HEAD
)
--
2.19.0.605.g01d371f741-goog
[PATCH v4 7/9] submodule: fetch in submodules git directory instead of in worktree
This patch started as a refactoring to make 'get_next_submodule' more
readable, but upon doing so, I realized that "git fetch" of the submodule
actually doesn't need to be run in the submodules worktree. So let's run
it in its git dir instead.
That should pave the way towards fetching submodules that are currently
not checked out.
This patch leaks the cp->dir in get_next_submodule, as any further
callback in run_processes_parallel doesn't have access to the child
process any more. In an early iteration of this patch, the function
get_submodule_repo_for directly returned the string containing the
git directory, which would be a better design choice for this patch.
However the next patch both fixes the memory leak of cp->dir and also has
a use case for using the full repository handle of the submodule, so
it makes sense to introduce the get_submodule_repo_for here already.
Signed-off-by: Stefan Beller
---
submodule.c | 51 +++--
t/t5526-fetch-submodules.sh | 7 -
2 files changed, 44 insertions(+), 14 deletions(-)
diff --git a/submodule.c b/submodule.c
index dd478ed70bf..3f791f22771 100644
--- a/submodule.c
+++ b/submodule.c
@@ -481,6 +481,12 @@ void prepare_submodule_repo_env(struct argv_array *out)
DEFAULT_GIT_DIR_ENVIRONMENT);
}
+static void prepare_submodule_repo_env_in_gitdir(struct argv_array *out)
+{
+ prepare_submodule_repo_env_no_git_dir(out);
+ argv_array_pushf(out, "%s=.", GIT_DIR_ENVIRONMENT);
+}
+
/* Helper function to display the submodule header line prior to the full
* summary output. If it can locate the submodule objects directory it will
* attempt to lookup both the left and right commits and put them into the
@@ -1227,6 +1233,29 @@ static int get_fetch_recurse_config(const struct
submodule *submodule,
return spf->default_option;
}
+static struct repository *get_submodule_repo_for(struct repository *r,
+const struct submodule *sub)
+{
+ struct repository *ret = xmalloc(sizeof(*ret));
+
+ if (repo_submodule_init(ret, r, sub)) {
+ /*
+* No entry in .gitmodules? Technically not a submodule,
+* but historically we supported repositories that happen to be
+* in-place where a gitlink is. Keep supporting them.
+*/
+ struct strbuf gitdir = STRBUF_INIT;
+ strbuf_repo_worktree_path(, r, "%s/.git", sub->path);
+ if (repo_init(ret, gitdir.buf, NULL)) {
+ strbuf_release();
+ return NULL;
+ }
+ strbuf_release();
+ }
+
+ return ret;
+}
+
static int get_next_submodule(struct child_process *cp,
struct strbuf *err, void *data, void **task_cb)
{
@@ -1234,12 +1263,11 @@ static int get_next_submodule(struct child_process *cp,
struct submodule_parallel_fetch *spf = data;
for (; spf->count < spf->r->index->cache_nr; spf->count++) {
- struct strbuf submodule_path = STRBUF_INIT;
- struct strbuf submodule_git_dir = STRBUF_INIT;
struct strbuf submodule_prefix = STRBUF_INIT;
const struct cache_entry *ce = spf->r->index->cache[spf->count];
- const char *git_dir, *default_argv;
+ const char *default_argv;
const struct submodule *submodule;
+ struct repository *repo;
struct submodule default_submodule = SUBMODULE_INIT;
if (!S_ISGITLINK(ce->ce_mode))
@@ -1274,16 +1302,12 @@ static int get_next_submodule(struct child_process *cp,
continue;
}
- strbuf_repo_worktree_path(_path, spf->r, "%s",
ce->name);
- strbuf_addf(_git_dir, "%s/.git", submodule_path.buf);
strbuf_addf(_prefix, "%s%s/", spf->prefix, ce->name);
- git_dir = read_gitfile(submodule_git_dir.buf);
- if (!git_dir)
- git_dir = submodule_git_dir.buf;
- if (is_directory(git_dir)) {
+ repo = get_submodule_repo_for(spf->r, submodule);
+ if (repo) {
child_process_init(cp);
- cp->dir = strbuf_detach(_path, NULL);
- prepare_submodule_repo_env(>env_array);
+ prepare_submodule_repo_env_in_gitdir(>env_array);
+ cp->dir = xstrdup(repo->gitdir);
cp->git_cmd = 1;
if (!spf->quiet)
strbuf_addf(err, "Fetching submodule %s%s\n",
@@ -1293,10 +1317,11 @@ static int get_next_submodule(struct child_process *cp,
argv_array_push(>args, default_argv);
argv_array_push(>args, "--submodule-prefix");
[PATCH v4 8/9] fetch: retry fetching submodules if needed objects were not fetched
Currently when git-fetch is asked to recurse into submodules, it dispatches
a plain "git-fetch -C " (with some submodule related options
such as prefix and recusing strategy, but) without any information of the
remote or the tip that should be fetched.
This works surprisingly well in some workflows (such as using submodules
as a third party library), while not so well in other scenarios, such
as in a Gerrit topic-based workflow, that can tie together changes
(potentially across repositories) on the server side. One of the parts
of such a Gerrit workflow is to download a change when wanting to examine
it, and you'd want to have its submodule changes that are in the same
topic downloaded as well. However these submodule changes reside in their
own repository in their own ref (refs/changes/).
Retry fetching a submodule by object name if the object id that the
superproject points to, cannot be found.
This retrying does not happen when the "git fetch" done at the
superproject is not storing the fetched results in remote
tracking branches (i.e. instead just recording them to
FETCH_HEAD) in this step. A later patch will fix this.
builtin/fetch used to only inspect submodules when they were fetched
"on-demand", as in either on/off case it was clear whether the submodule
needs to be fetched. However to know whether we need to try fetching the
object ids, we need to identify the object names, which is done in this
function check_for_new_submodule_commits(), so we'll also run that code
in case the submodule recursion is set to "on".
Signed-off-by: Stefan Beller
---
builtin/fetch.c | 9 +-
submodule.c | 185 ++--
t/t5526-fetch-submodules.sh | 16
3 files changed, 177 insertions(+), 33 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 0696abfc2a1..e3b03ad3bd3 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -707,8 +707,7 @@ static int update_local_ref(struct ref *ref,
what = _("[new ref]");
}
- if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
- (recurse_submodules != RECURSE_SUBMODULES_ON))
+ if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref(msg, ref, 0);
format_display(display, r ? '!' : '*', what,
@@ -723,8 +722,7 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(, >object.oid,
DEFAULT_ABBREV);
strbuf_addstr(, "..");
strbuf_add_unique_abbrev(, >new_oid,
DEFAULT_ABBREV);
- if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
- (recurse_submodules != RECURSE_SUBMODULES_ON))
+ if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref("fast-forward", ref, 1);
format_display(display, r ? '!' : ' ', quickref.buf,
@@ -738,8 +736,7 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(, >object.oid,
DEFAULT_ABBREV);
strbuf_addstr(, "...");
strbuf_add_unique_abbrev(, >new_oid,
DEFAULT_ABBREV);
- if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
- (recurse_submodules != RECURSE_SUBMODULES_ON))
+ if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref("forced-update", ref, 1);
format_display(display, r ? '!' : '+', quickref.buf,
diff --git a/submodule.c b/submodule.c
index 3f791f22771..05799362e05 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1127,8 +1127,12 @@ struct submodule_parallel_fetch {
int result;
struct string_list changed_submodule_names;
+ struct get_next_submodule_task **retry;
+ int retry_nr, retry_alloc;
};
-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0,
STRING_LIST_INIT_DUP }
+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \
+ STRING_LIST_INIT_DUP, \
+ NULL, 0, 0}
static void calculate_changed_submodule_paths(
struct submodule_parallel_fetch *spf)
@@ -1233,6 +1237,56 @@ static int get_fetch_recurse_config(const struct
submodule *submodule,
return spf->default_option;
}
+struct get_next_submodule_task {
+ struct repository *repo;
+ const struct submodule *sub;
+ unsigned free_sub : 1; /* Do we need to free the submodule? */
+ struct oid_array *commits;
+};
+
+static const struct submodule *get_default_submodule(const char *path)
+{
+ struct submodule *ret = NULL;
+ const char *name = default_name_or_path(path);
+
+ if (!name)
+ return NULL;
+
+ ret = xmalloc(sizeof(*ret));
+
Re: [PATCH 1/8] sha1-array: provide oid_array_filter
On Sat, Sep 22, 2018 at 5:58 AM Ævar Arnfjörð Bjarmason
wrote:
>
>
> On Fri, Sep 21 2018, Stefan Beller wrote:
>
> > +/*
> > + * Apply want to each entry in array, retaining only the entries for
> > + * which the function returns true. Preserve the order of the entries
> > + * that are retained.
> > + */
> > +void oid_array_filter(struct oid_array *array,
> > + for_each_oid_fn want,
> > + void *cbdata);
> > +
> > #endif /* SHA1_ARRAY_H */
>
> The code LGTM, but this comment should instead be an update to the API
> docs, see my recent 5cc044e025 ("get_short_oid: sort ambiguous objects
> by type, then SHA-1", 2018-05-10) for an addition of a new function to
> this API where I added some new docs.
ok will fix for consistency (this whole API is there).
Longer term (I thought) we were trying to migrate API docs
to headers instead?
Stefan
Re: git fetch behaves weirdely when run in a worktree
On Mon, 2018-09-24 at 17:17 +0200, Duy Nguyen wrote: > On Sun, Sep 23, 2018 at 10:19 PM Kaartic Sivaraam > wrote: > > Yes, some bugs. It behaves correctly for me. There must be something > strange that triggers this. What's your "git worktree list" (iow > anything strange there, duplicate worktrees perhaps)? Nothing seems strange in the list. $ git worktree list $COMMON_ROOT/git 01d371f741 (detached HEAD) $COMMON_ROOT/git-next cfa73bbfcb (detached HEAD) $COMMON_ROOT/git-next-build-automate 01d371f741 (detached HEAD) $COMMON_ROOT/git-pu 363c5c06bb (detached HEAD) Note: I sanitized the path in which the git worktrees (including the main worktree) is present as $COMMON_ROOT. > Also please try > "git fetch" again with GIT_TRACE=1 and GIT_TRACE_SETUP=1. Hopefully we > could catch something with that. $ GIT_TRACE_SETUP=1 GIT_TRACE=1 git fetch origin next 23:10:26.049785 trace.c:377 setup: git_dir: $COMMON_ROOT/git/.git/worktrees/git-next-build-automate 23:10:26.049868 trace.c:378 setup: git_common_dir: $COMMON_ROOT/git/.git 23:10:26.049901 trace.c:379 setup: worktree: $COMMON_ROOT/git-next-build-automate 23:10:26.049922 trace.c:380 setup: cwd: $COMMON_ROOT/git-next-build-automate 23:10:26.049941 trace.c:381 setup: prefix: (null) 23:10:26.049955 git.c:415 trace: built-in: git fetch origin next 23:10:26.051033 run-command.c:637 trace: run_command: git-remote-https origin https://github.com/git/git.git 23:10:28.366526 run-command.c:637 trace: run_command: git rev-list --objects --stdin --not --all --quiet 23:10:28.400979 run-command.c:637 trace: run_command: git rev-list --objects --stdin --not --all --quiet 23:10:28.402745 trace.c:377 setup: git_dir: $COMMON_ROOT/git/.git/worktrees/git-next-build-automate 23:10:28.402787 trace.c:378 setup: git_common_dir: $COMMON_ROOT/git/.git 23:10:28.402793 trace.c:379 setup: worktree: $COMMON_ROOT/git-next-build-automate 23:10:28.402798 trace.c:380 setup: cwd: $COMMON_ROOT/git-next-build-automate 23:10:28.402802 trace.c:381 setup: prefix: (null) 23:10:28.402815 git.c:415 trace: built-in: git rev-list --objects --stdin --not --all --quiet >From https://github.com/git/git * branch next -> FETCH_HEAD 23:10:28.437350 run-command.c:1553 run_processes_parallel: preparing to run up to 1 tasks 23:10:28.437481 run-command.c:1585 run_processes_parallel: done 23:10:28.437763 run-command.c:637 trace: run_command: git gc --auto 23:10:28.439608 trace.c:377 setup: git_dir: $COMMON_ROOT/git/.git/worktrees/git-next-build-automate 23:10:28.439655 trace.c:378 setup: git_common_dir: $COMMON_ROOT/git/.git 23:10:28.439667 trace.c:379 setup: worktree: $COMMON_ROOT/git-next-build-automate 23:10:28.439677 trace.c:380 setup: cwd: $COMMON_ROOT/git-next-build-automate 23:10:28.439687 trace.c:381 setup: prefix: (null) 23:10:28.439699 git.c:415 trace: built-in: git gc --auto HTH, Sivaraam
Re: Re*: [PATCH v3 0/5] Cleanup pass on special test setups
On 9/20/2018 2:43 PM, Junio C Hamano wrote:
Ben Peart writes:
This round has one code change based on feedback. Other changes are just
rewording commit messages.
Thanks. I think the only remaining issue is what to do with the
interaction between extra/additional error message that comes from
the updates in 3/5 and the test framework selftest in t.
-- >8 --
Subject: t: do not get self-test disrupted by environment warnings
The test framework test-lib.sh itself would want to give warnings
and hints, e.g. when it sees a deprecated environment variable is in
use that we want to encourage users to migrate to another variable.
The self-test of test framework done in t however do not expect
to see these warnings and hints, so depending on the settings of
environment variables, a running test may or may not produce these
messages to the standard error output, breaking the expectations of
self-test test framework does on itself. Here is what we see:
$ TEST_GIT_INDEX_VERSION=4 sh t-basic.sh -i -v
...
'err' is not empty, it contains:
warning: TEST_GIT_INDEX_VERSION is now GIT_TEST_INDEX_VERSION
hint: set GIT_TEST_INDEX_VERSION too during the transition period
not ok 5 - pretend we have a fully passing test suite
The following quick attempt to work it around does not work, because
some tests in t do want to see expected errors from the test
framework itself.
t/t-basic.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t-basic.sh b/t/t-basic.sh
index 850f651e4e..88c6ed4696 100755
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -88,7 +88,7 @@ _run_sub_test_lib_test_common () {
'
# Point to the t/test-lib.sh, which isn't in ../ as
usual
- . "\$TEST_DIRECTORY"/test-lib.sh
+ . "\$TEST_DIRECTORY"/test-lib.sh >/dev/null 2>&1
EOF
cat >>"$name.sh" &&
chmod +x "$name.sh" &&
There are a few possible ways to work this around:
* We could strip the warning: and hint: unconditionally from the
error output before the error messages are checked in the
self-test (helper functions check_sub_test_lib_test_err and
check_sub_test_lib_test); the problem with this approach is that
it will make it impossible to write self-tests to ensure that
right warnings and hints are given.
* We could force a sane environment settings before the test helper
_run_sub_test_lib_test_common dot-sources test-lib.sh; the
problem with this approach is that _run_sub_test_lib_test_common
now needs to be aware of what pairs of environment variables are
checked in test-lib.sh using check_var_migration helper.
The final patch I came up with is probably the solution that is
least bad. Set a variable to tell test-lib.sh that we are running
a self-test, so that various pieces in test-lib.sh can react to keep
the output stable.
This looks like a reasonable compromise to me. It's nice to give the
migration hints to end users so they know they need to update their
environments to reflect the required changes. On the other hand, we
don't want or need them to be triggered when we are running the self-test.
It would be nice to enable that automatically without the need for
another environment variable but I couldn't think of a good way to
accomplish that so I agree - this seems like the "least bad" solution. :-)
Thanks Junio
Signed-off-by: Junio C Hamano
---
t/t-basic.sh | 4
t/test-lib.sh| 8
2 files changed, 12 insertions(+)
diff --git a/t/t-basic.sh b/t/t-basic.sh
index 850f651e4e..52c02b7c7e 100755
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -87,6 +87,10 @@ _run_sub_test_lib_test_common () {
passing metrics
'
+ # Tell the framework that we are self-testing to make sure
+ # it yields a stable result.
+ GIT_TEST_FRAMEWORK_SELFTEST=t &&
+
# Point to the t/test-lib.sh, which isn't in ../ as usual
. "\$TEST_DIRECTORY"/test-lib.sh
EOF
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8ef86e05a3..364a11ea25 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -135,9 +135,17 @@ GIT_TRACE_BARE=1
export GIT_TRACE_BARE
check_var_migration () {
+ # the warnings and hints given from this helper depends
+ # on end-user settings, which will disrupt the self-test
+ # done on the test framework itself.
+ case "$GIT_TEST_FRAMEWORK_SELFTEST" in
+ t) return ;;
+ esac
+
old_name=$1 new_name=$2
eval "old_isset=\${${old_name}:+isset}"
eval "new_isset=\${${new_name}:+isset}"
+
case "$old_isset,$new_isset" in
isset,)
echo >&2 "warning: $old_name is now
Git Test Coverage Report (Tuesday, Sept 25)
In an effort to ensure new code is reasonably covered by the test suite,
we now have contrib/coverage-diff.sh to combine the gcov output from
'make coverage-test ; make coverage-report' with the output from 'git
diff A B' to discover _new_ lines of code that are not covered.
This report takes the output of these results after running on four
branches:
pu: 80e728fa913dc3a1165b6dec9a7afa6052a86325
jch: 0c10634844314ab89666ed0a1c7d36dde7ac9689
next: 76f2f5c1e34c4dbef1029e2984c2892894c444ce
master: fe8321ec057f9231c26c29b364721568e58040f7
master@{1}: 2d3b1c576c85b7f5db1f418907af00ab88e0c303
I ran the test suite on each of these branches on an Ubuntu Linux VM,
and I'm missing some dependencies (like apache, svn, and perforce) so
not all tests are run.
I submit this output without comment. I'm taking a look especially at my
own lines to see where coverage can be improved.
Thanks,
-Stolee
---
master@{1}..master:
builtin/remote.c
5025425dfff ( Shulhan 2018-09-13 20:18:33 +0700
864) return error(_("No such remote: '%s'"), name);
commit-reach.c
b67f6b26e35 (Derrick Stolee 2018-09-21 08:05:26 -0700
559) continue;
b67f6b26e35 (Derrick Stolee 2018-09-21 08:05:26 -0700
569) from->objects[i].item->flags |= assign_flag;
b67f6b26e35 (Derrick Stolee 2018-09-21 08:05:26 -0700
570) continue;
b67f6b26e35 (Derrick Stolee 2018-09-21 08:05:26 -0700
576) result = 0;
b67f6b26e35 (Derrick Stolee 2018-09-21 08:05:26 -0700
577) goto cleanup;
cat: compat#mingw.c.gcov: No such file or directory
ll-merge.c
d64324cb60e (Torsten Bögershausen 2018-09-12 21:32:02 +0200
379) marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
remote-curl.c
c3b9bc94b9b (Elijah Newren 2018-09-05 10:03:07 -0700
181) options.filter = xstrdup(value);
master..next:
fsck.c
fb8952077df (René Scharfe 2018-09-03 14:49:26 +
212) die_errno("Could not read '%s'", path);
midx.c
56ee7ff1565 (Derrick Stolee 2018-09-13 11:02:13 -0700
949) return 0;
cc6af73c029 (Derrick Stolee 2018-09-13 11:02:25 -0700
990) midx_report(_("failed to load pack-index for
packfile %s"),
cc6af73c029 (Derrick Stolee 2018-09-13 11:02:25 -0700
991) e.p->pack_name);
cc6af73c029 (Derrick Stolee 2018-09-13 11:02:25 -0700
992) break;
next..jch:
blame.c
a470beea39b (Nguyễn Thái Ngọc Duy 2018-09-21 17:57:21 +0200
113) !strcmp(r->index->cache[-1 - pos]->name, path))
a470beea39b (Nguyễn Thái Ngọc Duy 2018-09-21 17:57:21 +0200
272) int pos = index_name_pos(r->index, path, len);
a470beea39b (Nguyễn Thái Ngọc Duy 2018-09-21 17:57:21 +0200
274) mode = r->index->cache[pos]->ce_mode;
commit-graph.c
5cef295f283 (Derrick Stolee 2018-08-20 18:24:32 +
67) return 0;
20fd6d57996 (Derrick Stolee 2018-08-20 18:24:30 +
79) return 0;
config.c
4ee45bacad0 ( Ben Peart 2018-09-12 16:18:55 +
2301) if (is_bool)
4ee45bacad0 ( Ben Peart 2018-09-12 16:18:55 +
2302) return val ? 0 : 1;
4ee45bacad0 ( Ben Peart 2018-09-12 16:18:55 +
2304) return val;
diff.c
b78ea5fc357 (Nguyễn Thái Ngọc Duy 2018-09-21 17:57:19 +0200
4117) add_external_diff_name(o->repo, , other, two);
http-push.c
2abf3503854 (Nguyễn Thái Ngọc Duy 2018-09-21 17:57:38 +0200
1928) repo_init_revisions(the_repository, ,
setup_git_directory());
list-objects-filter-options.c
7c8a0cecc49 (Matthew DeVore 2018-09-21 13:32:04 -0700
55) if (errbuf) {
7c8a0cecc49 (Matthew DeVore 2018-09-21 13:32:04 -0700
56) strbuf_addstr(
7c8a0cecc49 (Matthew DeVore 2018-09-21 13:32:04 -0700
60) return 1;
ba72cca605f (Matthew DeVore 2018-09-21 13:32:03 -0700 86) if
(errbuf)
list-objects-filter.c
22e9b63e620 (Matthew DeVore 2018-09-21 13:32:02 -0700
47) BUG("unknown filter_situation: %d", filter_situation);
7c8a0cecc49 (Matthew DeVore 2018-09-21 13:32:04 -0700 100) default:
7c8a0cecc49 (Matthew DeVore 2018-09-21 13:32:04 -0700
101) BUG("unknown filter_situation: %d", filter_situation);
22e9b63e620 (Matthew DeVore 2018-09-21 13:32:02 -0700
152) BUG("unknown filter_situation: %d", filter_situation);
22e9b63e620 (Matthew DeVore 2018-09-21 13:32:02 -0700
257) BUG("unknown filter_situation: %d", filter_situation);
22e9b63e620 (Matthew DeVore 2018-09-21 13:32:02 -0700
438) BUG("invalid list-objects filter choice: %d",
list-objects.c
f447a499dbb (Matthew DeVore 2018-08-13 11:14:28 -0700
197)
Re: [PATCH v3 1/1] contrib: add coverage-diff script
"Derrick Stolee via GitGitGadget" writes:
> +files=$(git diff --name-only $V1 $V2 -- *.c)
You'd want to quote that *.c from the shell, i.e. either one of
these
files=$(git diff --name-only $V1 $V2 -- \*.c)
files=$(git diff --name-only $V1 $V2 -- '*.c')
otherwise you'd lose things like "builtin/am.c", I'd think.
> +
> +for file in $files
> +do
I know this is only for running in _our_ source tree, and we do not
have a source with $IFS in it, so I'd declare that this is OK. It
would be good to document that assumption in red capital letters at
the beginning of this loop, though ;-)
# Note: this script is only for our codebase and we rely on
# the fact that the pathnames of our source files do not
# have any funny characters---letting the shell split $files
# list at $IFS boundary is very much intentional, and not
# quoting "$file" in the code below also is. Don't imitate
# this in scripts that are meant to handle random end-user
# repositories!
for file in $files
do
...
> + git diff $V1 $V2 -- $file \
> + | diff_lines \
> + | sort >new_lines.txt
I do not see a strong reason why we would want to limit $V1 and $V2
to branch names and raw commit object names, and quoting them in dq
pair is a cheap fix to allow things like
$ contrib/coverage-diff.sh master 'pu^{/^### match next}'
so let's do so.
Could you cut lines _after_ typing a pipe and omit backslashes, i.e.
git diff "$V1" "$V2" -- $file |
diff_lines |
sort >new_lines.txt
It seems to be personal taste whether to indent the second and
subsequent lines; I do not care if you indent or if you align too
much either way (but I have moderate perference to align).
But I do not want to see people type unnecessary backslashes. This
is not limited to just this pipeline but elsewhere in the script.
> + if ! test -s new_lines.txt
> + then
> + continue
> + fi
> +
> + hash_file=$(echo $file | sed "s/\//\#/")
> + sed -ne '/#:/{
> + s/#://
> + s/:.*//
> + s/ //g
> + p
> + }' "$hash_file.gcov" \
> + | sort >uncovered_lines.txt
> +
> + comm -12 uncovered_lines.txt new_lines.txt \
> + | sed -e 's/$/\)/' \
> + | sed -e 's/^/\t/' \
Do you need two sed invocations for this, or would
sed -e 's/$/\)/' -e '/^//'
work as well? By the way """The meaning of an unescaped
immediately followed by any character other than '&', , a
digit, , or the delimiter character used for this command,
is unspecified."""[*1*] so '\t' on the replacement side is a no-no
in the portability department.
Reference. *1*
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html
> + >uncovered_new_lines.txt
> +
> + grep -q '[^[:space:]]' < uncovered_new_lines.txt && \
Lose the backslash at the end. The shell knows that you haven't
finished your sentence when it sees a line that ends with &&, || or
a pipe |, so there is no need to tell it redundantly that you have
more things to say with the backslash.
> + echo $file && \
> + git blame -c $file \
> + | grep -f uncovered_new_lines.txt
> +
> + rm -f new_lines.txt uncovered_lines.txt uncovered_new_lines.txt
> +done
Near the begininng (like just before the "for file in $files" loop),
you can probably have a trap to make sure these are removed upon
exit, e.g.
trap 'rm -f new_lines.txt uncovered_lines.txt uncovered_new_lines.txt' 0
Re: [PATCH] commit-reach: cleanups in can_all_from_reach...
On 9/25/2018 2:06 PM, Junio C Hamano wrote:
Derrick Stolee writes:
@@ -622,10 +623,7 @@ int can_all_from_reach_with_flag(struct object_array *from,
}
cleanup:
- for (i = 0; i < nr_commits; i++) {
- clear_commit_marks(list[i], RESULT);
- clear_commit_marks(list[i], assign_flag);
- }
+ clear_commit_marks_many(nr_commits, list, RESULT | assign_flag);
free(list);
for (i = 0; i < from->nr; i++)
base-commit: 4067a64672f9db8ca38d5a2682a7cdba7938c18b
This change looks good to me.
This is a tangent, but while re-reading clear_commit_marks() and its
helpers to refresh my memory, I found that the bottom-most helper in
the callchain was written in a very confusing way, but it is not a
fault of this clean-up. I however suspect that it would not help us
all that much to use clear_commit_marks_many() with its current
implementation. It first clears all commits on the first-parent
chain from each list[] element, while accumulating the parent
commits that are yet to be processed in a commit_list in LIFO order,
and then consumes these accumulated side parents the same way. We
probably would benefit by rewriting clear_commit_marks_many() to
traverse from all the tips given in list[] taking advantage of the
generation numbers, using a prio queue to manage the commits
yet-to-be-cleared, or something.
Another commit walk that could be improved by generation numbers? It's
like my bat-signal!
Thanks for pointing me in that direction. I'll take a look.
-Stolee
Re: [PATCH] commit-reach: cleanups in can_all_from_reach...
Derrick Stolee writes:
> @@ -622,10 +623,7 @@ int can_all_from_reach_with_flag(struct object_array
> *from,
> }
>
> cleanup:
> - for (i = 0; i < nr_commits; i++) {
> - clear_commit_marks(list[i], RESULT);
> - clear_commit_marks(list[i], assign_flag);
> - }
> + clear_commit_marks_many(nr_commits, list, RESULT | assign_flag);
> free(list);
>
> for (i = 0; i < from->nr; i++)
>
> base-commit: 4067a64672f9db8ca38d5a2682a7cdba7938c18b
This change looks good to me.
This is a tangent, but while re-reading clear_commit_marks() and its
helpers to refresh my memory, I found that the bottom-most helper in
the callchain was written in a very confusing way, but it is not a
fault of this clean-up. I however suspect that it would not help us
all that much to use clear_commit_marks_many() with its current
implementation. It first clears all commits on the first-parent
chain from each list[] element, while accumulating the parent
commits that are yet to be processed in a commit_list in LIFO order,
and then consumes these accumulated side parents the same way. We
probably would benefit by rewriting clear_commit_marks_many() to
traverse from all the tips given in list[] taking advantage of the
generation numbers, using a prio queue to manage the commits
yet-to-be-cleared, or something.
Re: [PATCH 1/1] read-cache: update index format default to v4
On Tue, Sep 25, 2018 at 7:30 AM Derrick Stolee wrote: > > On 9/25/2018 3:06 AM, Patrick Steinhardt wrote: > > On Mon, Sep 24, 2018 at 11:32:23PM +0200, SZEDER Gábor wrote: > >> On Mon, Sep 24, 2018 at 02:15:30PM -0700, Derrick Stolee via GitGitGadget > >> wrote: > >>> From: Derrick Stolee > >>> > >>> The index v4 format has been available since 2012 with 9d22778 > >>> "reach-cache.c: write prefix-compressed names in the index". Since > >>> the format has been stable for so long, almost all versions of Git > >>> in use today understand version 4, removing one barrier to upgrade > >>> -- that someone may want to downgrade and needs a working repo. > >> What about alternative implementations, like JGit, libgit2, etc.? > > Speaking of libgit2, we are able to read and write index v4 since > > commit c1b370e93 > > This is a good point, Szeder. > > Patrick: I'm glad LibGit2 is up-to-date with index formats. > > Unfortunately, taking a look (for the first time) at the JGit code > reveals that they don't appear to have v4 support. In > org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCache.java, the > DirCache.readFrom() method: lines 488-494, I see the following snippet: > > final int ver = NB.decodeInt32(hdr, 4); > boolean extended = false; > if (ver == 3) > extended = true; > else if (ver != 2) > throw new > CorruptObjectException(MessageFormat.format( > JGitText.get().unknownDIRCVersion, Integer.valueOf(ver))); > > It looks like this will immediately throw with versions other than 2 or 3. > > I'm adding Jonathan Nieder to CC so he can check with JGit people about > the impact of this change. JGit is used both on the server (which doesn't use index/staging area) as well as client side as e.g. an Eclipse integration, which would very much like to use the index. Adding Matthias Sohn as well, who is active in JGit and cares more about the client side than Googlers who only make use of the server side part of JGit. Stefan
Re: [PATCH 2/8] Add a place for (not) sharing stuff between worktrees
On Tue, Sep 25, 2018 at 9:55 AM Duy Nguyen wrote: > > And with that said, I wonder if the "local" part should be feature agnostic, > > or if we want to be "local" for worktrees, "local" for remotes, "local" > > for submodules (i.e. our own refs vs submodule refs). > > You lost me here. Yeah, me too after rereading. :P I think the "local" part always implies that there is a part that is not local and depending on the feature you call it remote or other worktree. When writing this comment I briefly wondered if we want to combine the local aspects of the various features. However the "local" part really depends on the feature (e.g. a ref on a different worktree is still local from the here/remote perspective or from the superproject/submodule perspective), so I think I was misguided. > > > think as long as the word "worktree" is in there, people would notice > > > the difference. > > > > That makes sense. But is refs/worktree shared or local? It's not quite > > obvious to me, as I could have refs/worktree//master > > instead when it is shared, so I tend to favor refs/local-worktree/ a bit > > more, but that is more typing. :/ > > OK I think mixing the two patches will different purposes messes you > (or me) up ;-) possible. > > refs/worktrees/xxx (and refs/main/xxx) are about visibility from other > worktrees. Or like Eric put it, they are simply aliases. These refs > are not shared because if they are, you can already see them without > new "ref mount points" like this. > > refs/worktree (previously refs/local) is also per-worktree but it's > specifically because you can't have per-worktree inside "refs/" (the > only exception so far is refs/bisect which is hard coded). You can > have refs outside "refs/" (like HEAD or FETCH_HEAD) and they will not > be shared, but they cannot be iterated while those inside refs/ can > be. This is more about deciding what to share and I believe is really > worktree-specific and only matters to _current_ worktree. > > Since refs/worktree is per-worktree, you can also view them from a > different worktree via refs/worktrees/. E.g. if you have > refs/worktree/foo then another worktree can see it via > refs/worktrees/xxx/refs/worktree/foo (besides pseudo refs like > refs/worktrees/xxx/HEAD) Ah. now I seem to understand, thanks for explaining.
Re: [PATCH] git help: promote 'git help -av'
On Tue, Sep 25, 2018 at 05:15:38PM +0200, Duy Nguyen wrote:
> On Mon, Sep 24, 2018 at 10:58 PM Junio C Hamano wrote:
> > I personally find "help -av" a bit too loud to my taste than plain
> > "-a", and more importantly, I look at "help -a" primarily to check
> > the last section "avaialble from elsewhere on your $PATH" to find
> > things like "clang-format", which I do not think is available
> > anywhere in "help -av", so I do not think "-av" can be promoted as
> > an upward-compatible replacement in its current form.
>
> Yep. I also thought "help -a" was denser but wasn't sure if it
> actually helps or not. Whenever I look at that block of commands, I
> end up searching anyway. For my use case, "help -a" could be better
> served with something like "git apropos".
>
> I think adding another section about external commands in "help -av"
> would address the "clang-format" stuff. With that, it's probably good
> enough to completely replace "help -a". It may also be good to list
> aliases there too in a separate section so you have "all you can type"
> in one (big) list.
Here's the patch that adds that external commands and aliases
sections. I feel that external commands section is definitely good to
have even if we don't replace "help -a". Aliases are more
subjective...
-- 8< --
diff --git a/builtin/help.c b/builtin/help.c
index 8d4f6dd301..23a34b36e7 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -38,7 +38,6 @@ static const char *html_path;
static int show_all = 0;
static int show_guides = 0;
static int show_config;
-static int verbose;
static unsigned int colopts;
static enum help_format help_format = HELP_FORMAT_NONE;
static int exclude_guides;
@@ -53,7 +52,6 @@ static struct option builtin_help_options[] = {
HELP_FORMAT_WEB),
OPT_SET_INT('i', "info", _format, N_("show info page"),
HELP_FORMAT_INFO),
- OPT__VERBOSE(, N_("print command description")),
OPT_END(),
};
@@ -437,14 +435,9 @@ int cmd_help(int argc, const char **argv, const char
*prefix)
if (show_all) {
git_config(git_help_config, NULL);
- if (verbose) {
- setup_pager();
- list_all_cmds_help();
- return 0;
- }
- printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
- load_command_list("git-", _cmds, _cmds);
- list_commands(colopts, _cmds, _cmds);
+ setup_pager();
+ list_all_cmds_help();
+ return 0;
}
if (show_config) {
diff --git a/help.c b/help.c
index 96f6d221ed..4a168230dc 100644
--- a/help.c
+++ b/help.c
@@ -98,7 +98,8 @@ static int cmd_name_cmp(const void *elem1, const void *elem2)
return strcmp(e1->name, e2->name);
}
-static void print_cmd_by_category(const struct category_description *catdesc)
+static void print_cmd_by_category(const struct category_description *catdesc,
+ int *longest_p)
{
struct cmdname_help *cmds;
int longest = 0;
@@ -124,6 +125,8 @@ static void print_cmd_by_category(const struct
category_description *catdesc)
print_command_list(cmds, mask, longest);
}
free(cmds);
+ if (longest_p)
+ *longest_p = longest;
}
void add_cmdname(struct cmdnames *cmds, const char *name, int len)
@@ -193,26 +196,6 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames
*excludes)
cmds->cnt = cj;
}
-static void pretty_print_cmdnames(struct cmdnames *cmds, unsigned int colopts)
-{
- struct string_list list = STRING_LIST_INIT_NODUP;
- struct column_options copts;
- int i;
-
- for (i = 0; i < cmds->cnt; i++)
- string_list_append(, cmds->names[i]->name);
- /*
-* always enable column display, we only consult column.*
-* about layout strategy and stuff
-*/
- colopts = (colopts & ~COL_ENABLE_MASK) | COL_ENABLED;
- memset(, 0, sizeof(copts));
- copts.indent = " ";
- copts.padding = 2;
- print_columns(, colopts, );
- string_list_clear(, 0);
-}
-
static void list_commands_in_dir(struct cmdnames *cmds,
const char *path,
const char *prefix)
@@ -285,29 +268,10 @@ void load_command_list(const char *prefix,
exclude_cmds(other_cmds, main_cmds);
}
-void list_commands(unsigned int colopts,
- struct cmdnames *main_cmds, struct cmdnames *other_cmds)
-{
- if (main_cmds->cnt) {
- const char *exec_path = git_exec_path();
- printf_ln(_("available git commands in '%s'"), exec_path);
- putchar('\n');
- pretty_print_cmdnames(main_cmds, colopts);
- putchar('\n');
- }
-
- if (other_cmds->cnt) {
- printf_ln(_("git commands
Re: [PATCH v2 3/3] transport.c: introduce core.alternateRefsPrefixes
Jeff King writes: > Right, I think that is totally fine for the current uses. I guess my > question was: do you envision cutting the interface down to only the > oids to bite us in the future? > > I was on the fence during past discussions, but I think I've come over > to the idea that the refnames actively confuse things. Alternates are sort-of repositories that you interact with via more normal transports like fetch or push, and at the object store level (i.e. the one that helps you build your local history) you do not really care what refnames other people use in their repository. E.g. it does not matter if a pull request to you asks you to pull their 'frotz' branch or 'nitfol' branch, as long as the work they did on that branch is what you expected them to do. And I think "I am aware that I can get to the objects that are reachable from these objects I can borrow from that alternate when I need them" is quite similar in spirit; the borrower has even less need to be aware of the refnames as there isn't even a need to "git pull" from it (at that only one single point, you would care what name they used in their pull request). So, I think we probably are better off without names.
Re: [PATCH 2/8] Add a place for (not) sharing stuff between worktrees
On Tue, Sep 25, 2018 at 6:24 PM Stefan Beller wrote: > > > That sounds dangerous to me. There is already a concept of > > > local and remote-tracking branches. So I would think that local > > > may soon become an overused word, (just like "index" today or > > > "recursive" to a lesser extend). > > > > > > Could this special area be more explicit? > > > (refs/worktree-local/ ? or after peeking at the docs below > > > refs/un-common/ ?) > > > > refs/un-common sounds really "uncommon" :D. If refs/local is bad, I > > guess we could go with either refs/worktree-local, refs/worktree, > > refs/private, refs/per-worktree... My vote is on refs/worktree. I > > refs/worktree sounds good to me (I do not object), but I am not > overly enthused either, as when I think further worktrees and > submodules are both features with a very similar nature in that > they touch a lot of core concepts in Git, but seem to be a niche > feature for the masses for now. I think the similarity is partly because submodule feature also has to manage worktrees. My view is at some point, this "git worktree" would be good enough that it can handle submodules as well (for the worktree part only of course) > For example I could think of submodules following this addressing > mode as well: submodule//master sounds similar to the > originally proposed worktree// convention. > For now it is not quite clear to me why you would want to have > access to the submodule refs in the superproject, but maybe > the use case will come later. Yeah. In theory we could "mount" the submodule ref store to a superproject's ref store. I think it may be needed just for the same reason it's needed for worktree: error reporting. If you peek into a submodule and say "HEAD has an error", the user will get confused whether it's superproject's HEAD or a submodule's HEAD. > And with that said, I wonder if the "local" part should be feature agnostic, > or if we want to be "local" for worktrees, "local" for remotes, "local" > for submodules (i.e. our own refs vs submodule refs). You lost me here. > > > think as long as the word "worktree" is in there, people would notice > > the difference. > > That makes sense. But is refs/worktree shared or local? It's not quite > obvious to me, as I could have refs/worktree//master > instead when it is shared, so I tend to favor refs/local-worktree/ a bit > more, but that is more typing. :/ OK I think mixing the two patches will different purposes messes you (or me) up ;-) refs/worktrees/xxx (and refs/main/xxx) are about visibility from other worktrees. Or like Eric put it, they are simply aliases. These refs are not shared because if they are, you can already see them without new "ref mount points" like this. refs/worktree (previously refs/local) is also per-worktree but it's specifically because you can't have per-worktree inside "refs/" (the only exception so far is refs/bisect which is hard coded). You can have refs outside "refs/" (like HEAD or FETCH_HEAD) and they will not be shared, but they cannot be iterated while those inside refs/ can be. This is more about deciding what to share and I believe is really worktree-specific and only matters to _current_ worktree. Since refs/worktree is per-worktree, you can also view them from a different worktree via refs/worktrees/. E.g. if you have refs/worktree/foo then another worktree can see it via refs/worktrees/xxx/refs/worktree/foo (besides pseudo refs like refs/worktrees/xxx/HEAD) > As we grow the worktree feature, do we ever expect the need to > reference the current worktree? > > For example when there is a ref "test" that could be unique per > repo and in the common area, so refs/heads/test would describe > it and "test" would get there in DWIM mode. > > But then I could also delete the common ref and recreate a "test" > ref in worktree A, in worktree B however DWIMming "test" could still > refer to A's "test" as it is unique (so far) in the repository. > And maybe I would want to check if test exists locally, so I'd > want to ask for "self/test" (with "self" == "B" as that is my cwd). You probably lost me again. In theory we must be able to detect ambiguity and stop DWIMing. If you want to be ambiguity-free, you specify full ref name, starting with "refs/" which should function like "self/" because worktree design so far is always about the current worktree's view. -- Duy
Re: [PATCH 3/8] refs: new ref types to make per-worktree refs visible to all worktrees
On Tue, Sep 25, 2018 at 8:49 AM Duy Nguyen wrote: > > On Tue, Sep 25, 2018 at 4:48 AM Stefan Beller wrote: > > > This patch also makes it possible to specify refs from one worktree in > > > another one, e.g. > > > > > > git log worktrees/foo/HEAD > > > > This has strong similarities to remote refs: > > Locally I may have a branch master, whose (stale local copy of its > > distributed) counterpart is named origin/master. > > If you think of each worktree as independent clones (which is more or > less true, the fact that they share ODB is more like an implementation > detail) then yes it's almost like remotes. Apart from the ODB and the refs subsystem, there is also the config space, which is shared (but you have sent out patches to have local config as well). So I would think worktrees are better than having two clones not just due to the shared ODB, but also due to the common config as then I have to setup my repo only once and can add/remove worktrees cheaply (in terms of "how much time do I need to spend to configure it as I need"). > > It is also possible to have a working tree named origin > > (just I like to name my worktree "git", when working on git.git), > > how do we differentiate between the neighbor-worktree > > "origin/master" and the remote-tracking branch "origin/master" ? > > Hmm.. I think you're thinking that origin/master could either mean > refs/worktrees/origin/master or refs/remotes/origin/master. I do not > think we're going to support expanding origin/master to > refs/worktrees/origin/master. This part about ref resolution did cross > my mind but I didn't see a good reason to support it. > > Even if we do support it, this is not even a new problem. If you have > refs/heads/origin/master and refs/remotes/origin/master now, we have > ref ambiguity anyway and a solution for this should handle > refs/worktrees/origin/master well if it comes into the picture. So once origin/master is overloaded, I would have to spell out refs/worktrees/origin/master and refs/remotes/origin/master to avoid confusing the DWIM machinery. Makes sense. > > How do we deal with that? > > main is accessed via worktrees/main/HEAD while the main worktree's > HEAD is accessed via main/HEAD (which is _not_ automatically expanded > to refs/worktrees/main/HEAD). But if it is, yes we need to detect > ambiguity and tell the user to specify full ref name, either > refs/main/HEAD or refs/worktrees/main/HEAD. Ah, I see. Now I actually understand the last paragraph of the commit message. Thanks for explaining! Stefan
Re: [PATCH 2/8] Add a place for (not) sharing stuff between worktrees
On Tue, Sep 25, 2018 at 8:36 AM Duy Nguyen wrote: > > On Tue, Sep 25, 2018 at 4:35 AM Stefan Beller wrote: > > > > On Sat, Sep 22, 2018 at 11:05 AM Nguyễn Thái Ngọc Duy > > wrote: > > > > > > When multiple worktrees are used, we need rules to determine if > > > something belongs to one worktree or all of them. Instead of keeping > > > adding rules when new stuff comes, have a generic rule: > > > > > > - Inside $GIT_DIR, which is per-worktree by default, add > > > $GIT_DIR/common which is always shared. New features that want to > > > share stuff should put stuff under this directory. > > > > So that /common is a directory and you have to use it specifically > > in new code? That would be easy to overlook when coming up > > with $GIT_DIR/foo for implementing the git-foo. > > There's no easy way out. I have to do _something_ if you want to share > $GIT_DIR/foo to all worktrees. Either we have to update path.c and add > "foo" which is not even an option for external commands, or we put > "foo" in a common place, e.g. $GIT_DIR/common/foo. > > > > - Inside refs/, which is shared by default except refs/bisect, add > > > refs/local/ which is per-worktree. We may eventually move > > > refs/bisect to this new location and remove the exception in refs > > > code. > > > > That sounds dangerous to me. There is already a concept of > > local and remote-tracking branches. So I would think that local > > may soon become an overused word, (just like "index" today or > > "recursive" to a lesser extend). > > > > Could this special area be more explicit? > > (refs/worktree-local/ ? or after peeking at the docs below > > refs/un-common/ ?) > > refs/un-common sounds really "uncommon" :D. If refs/local is bad, I > guess we could go with either refs/worktree-local, refs/worktree, > refs/private, refs/per-worktree... My vote is on refs/worktree. I refs/worktree sounds good to me (I do not object), but I am not overly enthused either, as when I think further worktrees and submodules are both features with a very similar nature in that they touch a lot of core concepts in Git, but seem to be a niche feature for the masses for now. For example I could think of submodules following this addressing mode as well: submodule//master sounds similar to the originally proposed worktree// convention. For now it is not quite clear to me why you would want to have access to the submodule refs in the superproject, but maybe the use case will come later. And with that said, I wonder if the "local" part should be feature agnostic, or if we want to be "local" for worktrees, "local" for remotes, "local" for submodules (i.e. our own refs vs submodule refs). > think as long as the word "worktree" is in there, people would notice > the difference. That makes sense. But is refs/worktree shared or local? It's not quite obvious to me, as I could have refs/worktree//master instead when it is shared, so I tend to favor refs/local-worktree/ a bit more, but that is more typing. :/ == As we grow the worktree feature, do we ever expect the need to reference the current worktree? For example when there is a ref "test" that could be unique per repo and in the common area, so refs/heads/test would describe it and "test" would get there in DWIM mode. But then I could also delete the common ref and recreate a "test" ref in worktree A, in worktree B however DWIMming "test" could still refer to A's "test" as it is unique (so far) in the repository. And maybe I would want to check if test exists locally, so I'd want to ask for "self/test" (with "self" == "B" as that is my cwd). Stefan
Re: bug in 'git describe'?
On Tue, Sep 25, 2018 at 6:05 PM Duy Nguyen wrote: > > On Tue, Sep 25, 2018 at 5:41 PM Sebastian Kuzminsky wrote: > > That behavior seems to me to be different from what the (2.11) manpage says: > > Good opportunity to improve the man page anyway even if Junio is > right. I agree that the section about "search strategy" is a bit > misleading because it does not mention anything about time stuff. If anybody's updating the man page, I think this is the commit that changed git-describe's search strategy: 80dbae03b0 (Chose better tag names in git-describe after merges. - 2007-01-10) -- Duy
Re: bug in 'git describe'?
On Tue, Sep 25, 2018 at 5:41 PM Sebastian Kuzminsky wrote: > That behavior seems to me to be different from what the (2.11) manpage says: Good opportunity to improve the man page anyway even if Junio is right. I agree that the section about "search strategy" is a bit misleading because it does not mention anything about time stuff. > > > it suffixes the tag name with the number of additional commits on top > > of the tagged object > > > And: > > > If multiple tags were found during the walk then the tag which has > > the fewest commits different from the input commit-ish will be > > selected and output. Here fewest commits different is defined as the > > number of commits which would be shown by git log tag..input will be > > the smallest number of commits possible. -- Duy
Re: [PATCH] worktree: add per-worktree config files
On Mon, Sep 24, 2018 at 4:21 PM Taylor Blau wrote:
> > +cmp_config() {
> > + if [ "$1" = "-C" ]; then
> > + shift &&
> > + GD="-C $1" &&
> > + shift
> > + else
> > + GD=
> > + fi &&
> > + echo "$1" >expected &&
> > + shift &&
> > + git $GD config "$@" >actual &&
> > + test_cmp expected actual
> > +}
>
> This cmp_config seems generally useful, perhaps beyond t2029. What do
> you think about putting it in t/test-lib-functions.sh instead?
Good point. t1300 (and I think some t13xx) does the same. Other tests also do
test "$(git config...)" = blah
which can also use this function to have better error reporting when
things go wrong.
--
Duy
Re: [PATCH 3/8] refs: new ref types to make per-worktree refs visible to all worktrees
On Tue, Sep 25, 2018 at 4:48 AM Stefan Beller wrote: > > This patch also makes it possible to specify refs from one worktree in > > another one, e.g. > > > > git log worktrees/foo/HEAD > > This has strong similarities to remote refs: > Locally I may have a branch master, whose (stale local copy of its > distributed) counterpart is named origin/master. If you think of each worktree as independent clones (which is more or less true, the fact that they share ODB is more like an implementation detail) then yes it's almost like remotes. > It is also possible to have a working tree named origin > (just I like to name my worktree "git", when working on git.git), > how do we differentiate between the neighbor-worktree > "origin/master" and the remote-tracking branch "origin/master" ? Hmm.. I think you're thinking that origin/master could either mean refs/worktrees/origin/master or refs/remotes/origin/master. I do not think we're going to support expanding origin/master to refs/worktrees/origin/master. This part about ref resolution did cross my mind but I didn't see a good reason to support it. Even if we do support it, this is not even a new problem. If you have refs/heads/origin/master and refs/remotes/origin/master now, we have ref ambiguity anyway and a solution for this should handle refs/worktrees/origin/master well if it comes into the picture. > As the remote tracking branches are shared between all > worktree there is no need to differentiate between a > local-worktree remote tracking branch and a > neighbor-worktree remote tracking branch. > > Now that you introduce the magic main working tree, > we also need to disallow working trees to be named "main", > i.e. > $ git worktree add main HEAD > > produces > > $ ls .git/worktrees/ > main > > How do we deal with that? main is accessed via worktrees/main/HEAD while the main worktree's HEAD is accessed via main/HEAD (which is _not_ automatically expanded to refs/worktrees/main/HEAD). But if it is, yes we need to detect ambiguity and tell the user to specify full ref name, either refs/main/HEAD or refs/worktrees/main/HEAD. -- Duy
Re: bug in 'git describe'?
On 9/24/18 4:24 PM, Junio C Hamano wrote: Sebastian Kuzminsky writes: I've got two tiny git repos whose commit graphs are identical, but where 'git describe' gives different results. ... The histories differ only in the timestamps of the commits... describe does take the commit timestamps into account, so it is expected you would get different results out of an otherwise identically looking graph. Thanks for that confirmation. That behavior seems to me to be different from what the (2.11) manpage says: it suffixes the tag name with the number of additional commits on top of the tagged object And: If multiple tags were found during the walk then the tag which has the fewest commits different from the input commit-ish will be selected and output. Here fewest commits different is defined as the number of commits which would be shown by git log tag..input will be the smallest number of commits possible. All that said, if you consider this "working as expected" then i'm content to let the matter drop. -- Sebastian Kuzminsky
Re: [PATCH 2/8] Add a place for (not) sharing stuff between worktrees
On Tue, Sep 25, 2018 at 4:35 AM Stefan Beller wrote: > > On Sat, Sep 22, 2018 at 11:05 AM Nguyễn Thái Ngọc Duy > wrote: > > > > When multiple worktrees are used, we need rules to determine if > > something belongs to one worktree or all of them. Instead of keeping > > adding rules when new stuff comes, have a generic rule: > > > > - Inside $GIT_DIR, which is per-worktree by default, add > > $GIT_DIR/common which is always shared. New features that want to > > share stuff should put stuff under this directory. > > So that /common is a directory and you have to use it specifically > in new code? That would be easy to overlook when coming up > with $GIT_DIR/foo for implementing the git-foo. There's no easy way out. I have to do _something_ if you want to share $GIT_DIR/foo to all worktrees. Either we have to update path.c and add "foo" which is not even an option for external commands, or we put "foo" in a common place, e.g. $GIT_DIR/common/foo. > > - Inside refs/, which is shared by default except refs/bisect, add > > refs/local/ which is per-worktree. We may eventually move > > refs/bisect to this new location and remove the exception in refs > > code. > > That sounds dangerous to me. There is already a concept of > local and remote-tracking branches. So I would think that local > may soon become an overused word, (just like "index" today or > "recursive" to a lesser extend). > > Could this special area be more explicit? > (refs/worktree-local/ ? or after peeking at the docs below > refs/un-common/ ?) refs/un-common sounds really "uncommon" :D. If refs/local is bad, I guess we could go with either refs/worktree-local, refs/worktree, refs/private, refs/per-worktree... My vote is on refs/worktree. I think as long as the word "worktree" is in there, people would notice the difference. -- Duy
Re: [PATCH] git help: promote 'git help -av'
On Mon, Sep 24, 2018 at 10:58 PM Junio C Hamano wrote: > I personally find "help -av" a bit too loud to my taste than plain > "-a", and more importantly, I look at "help -a" primarily to check > the last section "avaialble from elsewhere on your $PATH" to find > things like "clang-format", which I do not think is available > anywhere in "help -av", so I do not think "-av" can be promoted as > an upward-compatible replacement in its current form. Yep. I also thought "help -a" was denser but wasn't sure if it actually helps or not. Whenever I look at that block of commands, I end up searching anyway. For my use case, "help -a" could be better served with something like "git apropos". I think adding another section about external commands in "help -av" would address the "clang-format" stuff. With that, it's probably good enough to completely replace "help -a". It may also be good to list aliases there too in a separate section so you have "all you can type" in one (big) list. -- Duy
Re: [PATCH v5 8/8] Documentation/config: add odb..promisorRemote
On Tue, Sep 25, 2018 at 1:54 PM Christian Couder wrote: > > From: Christian Couder > > Signed-off-by: Junio C Hamano > --- > Documentation/config.txt | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/config.txt b/Documentation/config.txt > index ad0f4510c3..9df988adb9 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -2655,6 +2655,11 @@ This setting can be overridden with the > `GIT_NOTES_REWRITE_REF` > environment variable, which must be a colon separated list of refs or > globs. > > +odb..promisorRemote:: > + The name of a promisor remote. For now promisor remotes are > + the only kind of remote object database (odb) that is > + supported. > + Nit. If this is the beginning of "odb" section in config.txt, maybe move this to odb-config.txt and add include::odb-config.txt[] here since we're moving towards splitting config.txt for easier maintenance. -- Duy
Subtree question and possible issue
We've got a fairly large codebase that's been through two previous source control systems, and recently converted to git. We have shared modules (but with customization) between projects, so subtrees looked like a good approach to sharing the code between projects. I've encountered a problem pushing commits back to the subtree repo which generated a question and maybe exposed an issue, though the issue could be with our repo for all I know. [first attempt 2.16.0 (windows), retried with same results 2.19.0 (windows)] Easier one first. Repo is set up to include code from another repo via subtree. We push code to the subtree using split --rejoin and get a commit linking the two together. New work is done on a branch that has a parent *before* the rejoin, and then merges back *after* the rejoin. When we later want to sync code using split --rejoin, am I right in believing that commit will appear parentless in the subtree repo, since the split will not process any commits which were reachable from the rejoin? If that's right, is there any workaround other than --ignore-rejoins? Doing the split already takes quite a while, and if we don't get the benefit of rejoins, that process will only grow longer. Harder part now. The actual problem I encountered was the remote claiming that my push couldn't be accepted because it was behind the remote HEAD of the branch. After looking closer at the local split, I discovered that the rejoin commit was not part of the new commit graph, which meant the commits were completely independent of the remote branch (hence the rejection). I reran the script with debug info turned on, and found the following result which was suspicious: Processing commit: c3630d64ec64c56b3395bb9df573031d770803af parents: 8a1d1a97bfd04a77b2cd624038f64a256753fa09 24b59e2a5f198d97a132a1c3080321a7238e176f newparents: 7da344d52155ec5cc3c67dde3577c6a99510ad05 224fa84a093a3b0fc801d0ee1a2f7732a94e3f98 tree is: 4b26d4dbe94c2ca43a4faa6c8e09f567400752d5 newrev is: 224fa84a093a3b0fc801d0ee1a2f7732a94e3f98 This commit is the merge which links the parentless new-work commits (merged as 224fa8) to the rejoin commit (accessible through 7da344). This particular commit should have been parented by both, but instead the script considered the two parents to be identical and simplified the graph to only use one. After digging into the script, it seems that this is because the tree hashes of the commits are the same. I've run a similar command to the one in the script to evaluate the hash information for both commits, including both tree hashes and commit hashes. There are some duplicate lines between the two, but they are clearly different commits with different histories. I've uploaded that output here, flagging the duplicate lines for convenience: https://gist.github.com/FoxFireX/41328a87bceeb3542c7a968348a92fa5 To further test this, I ran the script directly in (windows) bash, with the -x debug flag enabled on the script. It shows the comparison of the tree hashes, which is causing it to lose the parent that would link the branches together. That output is available here: https://gist.github.com/FoxFireX/d6b254d0f01af6533874eff2bb9ac135 I also went down the path of running the split with --ignore-joins (which I may need to do due to the first question anyway) but that process ended up creating an incorrect history that doesn't match the existing subtree, including a variety of commits which are completely unrelated to the subtree prefix. I haven't fully gone through the troubleshooting I want there, but may come back with other questions as I work through that process. So, am I right about branching from pre-rejoin causing ugly commits in the subtree later? And what can I do about this split that refuses to properly join back up? Any help or insight would be most appreciated. -- Roger Strain
Re: [PATCH 1/1] read-cache: update index format default to v4
On 9/25/2018 3:06 AM, Patrick Steinhardt wrote: On Mon, Sep 24, 2018 at 11:32:23PM +0200, SZEDER Gábor wrote: On Mon, Sep 24, 2018 at 02:15:30PM -0700, Derrick Stolee via GitGitGadget wrote: From: Derrick Stolee The index v4 format has been available since 2012 with 9d22778 "reach-cache.c: write prefix-compressed names in the index". Since the format has been stable for so long, almost all versions of Git in use today understand version 4, removing one barrier to upgrade -- that someone may want to downgrade and needs a working repo. What about alternative implementations, like JGit, libgit2, etc.? Speaking of libgit2, we are able to read and write index v4 since commit c1b370e93 This is a good point, Szeder. Patrick: I'm glad LibGit2 is up-to-date with index formats. Unfortunately, taking a look (for the first time) at the JGit code reveals that they don't appear to have v4 support. In org.eclipse.jgit/src/org/eclipse/jgit/dircache/DirCache.java, the DirCache.readFrom() method: lines 488-494, I see the following snippet: final int ver = NB.decodeInt32(hdr, 4); boolean extended = false; if (ver == 3) extended = true; else if (ver != 2) throw new CorruptObjectException(MessageFormat.format( JGitText.get().unknownDIRCVersion, Integer.valueOf(ver))); It looks like this will immediately throw with versions other than 2 or 3. I'm adding Jonathan Nieder to CC so he can check with JGit people about the impact of this change. Thanks, -Stolee
