* [PATCH 2/2] Backup the array passed to fetch_pack so we can free items
From: Shawn O. Pearce @ 2007-09-14 22:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
fetch_pack() can call remove_duplicates() on its input array and
this will possibly overwrite an earlier entry with a later one if
there are any duplicates in the input array. In such a case the
caller here might then attempt to free an item multiple times as
it goes through its cleanup.
I also forgot to free the heads array we pass down into fetch_pack()
when I introduced the allocation of it in this function during my
builtin-fetch cleanup series. Better free it while we are here
working on related memory management fixes.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
transport.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/transport.c b/transport.c
index 0882edd..0338ed4 100644
--- a/transport.c
+++ b/transport.c
@@ -311,6 +311,7 @@ static int fetch_refs_via_pack(struct transport *transport,
{
struct git_transport_data *data = transport->data;
char **heads = xmalloc(nr_heads * sizeof(*heads));
+ char **origh = xmalloc(nr_heads * sizeof(*origh));
struct ref *refs;
char *dest = xstrdup(transport->url);
struct fetch_pack_args args;
@@ -329,11 +330,13 @@ static int fetch_refs_via_pack(struct transport *transport,
setup_fetch_pack(&args);
for (i = 0; i < nr_heads; i++)
- heads[i] = xstrdup(to_fetch[i]->name);
+ origh[i] = heads[i] = xstrdup(to_fetch[i]->name);
refs = fetch_pack(dest, nr_heads, heads, &transport->pack_lockfile);
for (i = 0; i < nr_heads; i++)
- free(heads[i]);
+ free(origh[i]);
+ free(origh);
+ free(heads);
free_refs(refs);
free(dest);
return 0;
--
1.5.3.1.921.g8c3b
^ permalink raw reply related
* [RESEND PATCH 1/2] Fix builtin-fetch memory corruption by not overstepping array
From: Shawn O. Pearce @ 2007-09-14 22:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
A long time ago Junio added this line to always ensure that the
output array created by remove_duplicates() had a NULL as its
terminating node. Today none of the downstream consumers of this
array care about a NULL terminator; they only pay attention to the
size of the array (as indicated by nr_heads). In (nearly?) all
cases passing a NULL element will cause SIGSEGV failures. So this
NULL terminal is not actually necessary.
Unfortunately we cannot continue to NULL terminate the array at
this point as the array may only have been allocated large enough
to match the input of nr_heads. If there are no duplicates than
we would be trying to store NULL into heads[nr_heads] and that may
be outside of the array.
My recent series to cleanup builtin-fetch changed the allocation of
the heads array from 256 entries to exactly nr_heads thus ensuring
we were always overstepping the array and causing memory corruption.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
*** RESEND TO CORRECT FROM+SBO ***
Sorry Junio. I realized too late that the headers were wrong.
That's what I get for not configuring ~/.gitconfig on my amd64
Linux box. ;-)
builtin-fetch-pack.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index b0936cc..2977a94 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -654,7 +654,6 @@ static int remove_duplicates(int nr_heads, char **heads)
heads[dst] = heads[src];
dst++;
}
- heads[dst] = 0;
return dst;
}
--
1.5.3.1.921.g8c3b
^ permalink raw reply related
* Re: [RESEND PATCH 1/2] Fix builtin-fetch memory corruption by not overstepping array
From: Shawn O. Pearce @ 2007-09-14 23:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20070914225953.GC16512@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> wrote:
> Unfortunately we cannot continue to NULL terminate the array at
> this point as the array may only have been allocated large enough
> to match the input of nr_heads. If there are no duplicates than
> we would be trying to store NULL into heads[nr_heads] and that may
> be outside of the array.
...
> diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
> index b0936cc..2977a94 100644
> --- a/builtin-fetch-pack.c
> +++ b/builtin-fetch-pack.c
> @@ -654,7 +654,6 @@ static int remove_duplicates(int nr_heads, char **heads)
> heads[dst] = heads[src];
> dst++;
> }
> - heads[dst] = 0;
> return dst;
> }
>
This fortunately does not appear to be a bug in `maint`, `master`,
or `next`. It appears to be isolated to the builtin-fetch topic.
The builtin-fetch topic changed fetch-pack to be builtin-fetch-pack
and in doing so changed things such that the heads array might not
have room for this final entry.
In maint/master/next the heads array is actually backed by the
argv passed into main by the kernel, and that does have an existing
NULL terminator, which the above "heads[dst] = 0" is simply going
to overwrite if "heads == nr_heads".
--
Shawn.
^ permalink raw reply
* Re: [PATCH] git-commit: Allow partial commit of file removal.
From: Junio C Hamano @ 2007-09-14 23:34 UTC (permalink / raw)
To: Gerrit Pape; +Cc: Martin Koegler, git
In-Reply-To: <7vzlzqieko.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Gerrit Pape <pape@smarden.org> writes:
>
>> On Wed, Sep 12, 2007 at 04:04:22PM -0700, Junio C Hamano wrote:
>>> * In my earlier reply to Gerrit, I hinted that we need to
>>> update the pathspec semantics in ls-tree to properly fix this
>>> issue. I cheated here and have ls-files apply its pathspec
>>> semantics to the entries from HEAD as well.
>>
>> This fixes the problem reported through http://bugs.debian.org/437817
>> just fine, thanks. Is this an interims-fix, or should the new option
>> be documented?
>
> I honestly am not convinced it is the right fix. It has a few
> holes in the logic.
>
> Most notably, I think "git rm --cached A; git commit A" would
> not work.
I managed to convince myself that not committing the removal of
A in that case is a _good_ thing, unless somebody comes up with
a good counterexample this will most likely go to 'master' over
the weekend and then to 'maint'.
Any partial commit "git commit <paths>..." is saying:
I might have changed stuff in the index and also have
changes in the working tree. But I do not care about
the changes between HEAD and the index. Honestly, I do
not understand the index at all, and I do not care about
what I staged earlier to the named paths either. Take
the current state of these paths from my work tree and
make a commit relative to the HEAD.
So, if you do:
$ edit new-file old-file
$ rm gone-file
$ git rm missing-file
$ git rm --cached disappeared-file
$ git add new-file ;# was not in HEAD
$ edit new-file old-file
Then:
$ git commit new-file old-file
honors what is in the work tree and picks up the later edits,
largely ignoring the changes to the index.
Removal should work the same way to be consistent.
$ git commit gone-file
$ git commit missing-file
$ git commit disappeared-file
should remove the former two but leave the last one alone, as
you _do_ still have disappeared-file in the work tree.
As it happens, the version I sent will error out on the last one
(disappeared-file case). I think the following patch on top of
it "fixes" it, by adding it back to the index and including its
contents in the resulting commit.
---
diff --git a/git-commit.sh b/git-commit.sh
index 5ea3fd0..bb113e8 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -404,7 +404,7 @@ t,)
(
GIT_INDEX_FILE="$NEXT_INDEX"
export GIT_INDEX_FILE
- git update-index --remove --stdin
+ git update-index --add --remove --stdin
) || exit
;;
esac
^ permalink raw reply related
* Re: [RESEND PATCH 1/2] Fix builtin-fetch memory corruption by not overstepping array
From: Junio C Hamano @ 2007-09-14 23:43 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git
In-Reply-To: <20070914225953.GC16512@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> writes:
> *** RESEND TO CORRECT FROM+SBO ***
>
> Sorry Junio. I realized too late that the headers were wrong.
> That's what I get for not configuring ~/.gitconfig on my amd64
> Linux box. ;-)
Actually this might have been a good thing, according to Linus.
It is an evidence that you built and tested that change on the
"blink" box not your usual box.
^ permalink raw reply
* Re: What's cooking in git.git (topics)
From: Johannes Schindelin @ 2007-09-14 23:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v1wd1d0le.fsf@gitster.siamese.dyndns.org>
Hi,
On Fri, 14 Sep 2007, Junio C Hamano wrote:
> * cr/reset (Fri Sep 14 01:19:30 2007 -0700) 5 commits
> + Simplify cache API
> + An additional test for "git-reset -- path"
> + Make "git reset" a builtin.
> + Move make_cache_entry() from merge-recursive.c into read-cache.c
> + Add tests for documented features of "git reset".
>
> I found "git reset commit paths..." had problem in this series,
> which was why jc/cachetree is merged into this topic to fix it.
> Hopefully we can put this in 'master' soon, after giving it
> another and final round of eyeballing.
I think this is my bug. The initial reset-with-paths functionality in the
builtin reset came out of my feather... Sorry!
Ciao,
Dscho
^ permalink raw reply
* Re: Data Integrity & un-Commited Branches
From: Brian Scott Dobrovodsky @ 2007-09-15 0:40 UTC (permalink / raw)
To: git
In-Reply-To: <7vk5qtd3le.fsf@gitster.siamese.dyndns.org>
It was a misunderstanding of Git's work flow. By switching from 'an
un-committed demo' to a previously committed master: I was expecting
Git to give me the content last commited to master while at the same
time preserving(without having to commit) the changes made in demo.
Intuitively, this is how I expected Git to function.
Indeed, I read through the Crash Courses: 'Git for everyone' & 'Git
for SVN users'.
--
Brian Scott Dobrovodsky
^ permalink raw reply
* Mailing patch series'
From: Russ Brown @ 2007-09-15 1:06 UTC (permalink / raw)
To: git
Hi,
I've noticed the method of mailing series' of patches on this mailing
list, and I'd like to know a little more about how it's done, as I'm
considering how well it might work for us as a workflow and review process.
Particularly, where does the series of patches come from? Is this a
usage scenario for stacked git (something else I don't fully understand
the rationale behind as yet)? I'm imagining the developer has a local
branch to which he commits his changes, and then rebases resulting in
his branches being at the end of the local branch. How are they then
extracted and mailed out?
My next question, is supposing that the patches are reviewed and changes
suggested. How does the developer then go about amending, say the second
patch and getting the subsequent ones rebased off that? I'm assuming
there's a nice clever way of doing it that doesn't involve manually
messing with individual patch files etc.
Please excuse my ignorance: I'm still getting my head around this. Once
I do that I'm usually away and happy, but it takes a while for that
'click' moment to hit me sometimes :)
Thanks.
--
Russ
^ permalink raw reply
* Re: Mailing patch series'
From: Shawn O. Pearce @ 2007-09-15 2:35 UTC (permalink / raw)
To: Russ Brown; +Cc: git
In-Reply-To: <46EB301B.8050602@gmail.com>
Russ Brown <pickscrape@gmail.com> wrote:
> I've noticed the method of mailing series' of patches on this mailing
> list, and I'd like to know a little more about how it's done, as I'm
> considering how well it might work for us as a workflow and review process.
>
> Particularly, where does the series of patches come from?
Usually from Git itself, as in each Git commit is turned into its
own email message by `git format-patch` which can then be sent by
`git send-email` or your favorite mail client.
> Is this a
> usage scenario for stacked git (something else I don't fully understand
> the rationale behind as yet)?
Yes, its one of them. When you are editing a series of patches that
you want someone to read as a linear "conversation" it can help to
use StGIT as you can reorder the commits (messages/patches) at will
and edit any of them at any time. guilt is another excellent tool
that also can be used here.
I actually just use core Git itself with `git rebase -i` when I need
to perform ordering/editing before sending. I like the interface it
offers, its fast enough for my needs, and well, I'm just so used to
the bare plumbing of Git that I think in terms of the DAG operators.
I've heard wonderful things about both StGIT and guilt, but they
aren't this particular developer's cup of tea.
> I'm imagining the developer has a local
> branch to which he commits his changes, and then rebases resulting in
> his branches being at the end of the local branch. How are they then
> extracted and mailed out?
git-format-patch takes one of two variations of arguments: either
a single commit or a range of commits. The most common usage is
in the single commit case.
When given a single commit git-format-patch creates a message for
each commit on the current branch that is after the supplied commit.
The second case (range of commits) it creates a message for each
commit in that range.
So when I start developing a topic I branch off Junio's master,
which I happen to fetch into refs/remotes/jc/master (as I had
previously done `git remote add jc $url`):
git checkout -b fetch-pack-fixes jc/master
jc/master
fetch-patch-fixes
/
-o-o-o-B
Then I do my development... and rebase against Junio before sending:
git fetch jc
fetch-patch-fixes
/
-o-o-o-B-m1-m2-m3-m4-m5 jc/master
\ /
o-o-o-o-o-o-o-o-o-C
git rebase jc/master
-o-o-o-B jc/master fetch-patch-fixes
\ / /
o-o-o-o-o-o-o-o-o-C-m1'-m2'-m3'-m4'-m5'
Now I retest my series and send it to Junio:
git format-patch --stdout jc/master >mbox
mutt -f mbox
When mutt (my preferred mail client) starts it is loading a mailbox
with one email message per commit. Only the commits on my current
branch (m1'-m5') that don't appear in jc/master (my tracking branch
of Junio's master) will be formatted into mbox. That is only my
work that I haven't sent yet.
I (re)review each commit and then a macro sends the message on
its way. To help me preaddress the emails I have the following
in .git/config:
[format]
headers = "To: Junio C Hamano <gitster@pobox.com>\n"
headers = "Cc: git@vger.kernel.org\n"
That way the messages have proper To/Cc lines automatically entered
and the mutt macro is really just to trigger sending the message.
Finally I leave that branch alone. If I want to make further changes
I actually fork off it and start a new branch:
git checkout -b fetch-pack-fixes2 fetch-pack-fixes
As now when I send this new branch I can use:
git format-patch --stdout fetch-pack-fixes >mbox
mutt -f mbox
To send only the commits since the last series that I sent. If my
work is worthy these commits will eventually come back during a
later fetch from Junio. I look for my work using `git log` and
`git cherry` and delete the branches that Junio has fully merged in.
I don't actually quite use all of those commands by hand; I have a
proper wrapper script that I've put together to automate remembering
what I last sent, defaulting to "jc/master", and to run the pipeline
of format-patch and mutt.
> My next question, is supposing that the patches are reviewed and changes
> suggested. How does the developer then go about amending, say the second
> patch and getting the subsequent ones rebased off that? I'm assuming
> there's a nice clever way of doing it that doesn't involve manually
> messing with individual patch files etc.
Use StGIT, guilt, or `git rebase -i jc/master` to go back and edit
the necessary patches in the series, then resubmit the entire series.
Or just the changed ones and the later ones, depending on what got
merged upstream already.
If you edit something in a particular change you can cause
merge conflicts when the other commits are pushed on top/picked
(terminology depending on your tool of choice). But you should
then get a merge conflict and be able to fix it up.
--
Shawn.
^ permalink raw reply
* Re: Data Integrity & un-Commited Branches
From: Shawn O. Pearce @ 2007-09-15 2:51 UTC (permalink / raw)
To: Brian Scott Dobrovodsky; +Cc: git
In-Reply-To: <2a8a071a0709141740l144b60aevdfec2b6cdab8bb60@mail.gmail.com>
Brian Scott Dobrovodsky <brian@pontech.com> wrote:
> It was a misunderstanding of Git's work flow. By switching from 'an
> un-committed demo' to a previously committed master: I was expecting
> Git to give me the content last commited to master while at the same
> time preserving(without having to commit) the changes made in demo.
> Intuitively, this is how I expected Git to function.
You aren't the only one.
Several of my day-job coworkers have also thought the same thing.
Only they use git-gui, and have never read any of the Git docs.
Because nobody ever reads the docs. Nope, not if you can just dial
my extension and browbeat me into giving you an answer to your most
urgent question. :-\
My point is just that some people actually assume that work done
while having one branch checked out is related to that branch and
that branch alone and that switching a branch should put that work
on hold. Unfortunately for me some of these people at day-job have
also just assumed Git can read their mind and forget to switch
branches at the proper times, resulting in unrelated work mashed
together for days straight (and criss-crossed merge to hell and back)
before they call me and say "MAKEITWORKNOW".
</rant>
It isn't unreasonable to want Git to save uncommitted work for the
current branch and then you switch to another, ending up with a
clean working directory when you finally get there. Today we have
git-stash to help you with this, but I'm thinking maybe we want to
connect git-checkout with it?
I see `-s` isn't used as an option yet. What about:
$ git init
$ echo master >file
$ git add file && git commit -m initial
$ git checkout -b demo ; # switch to demo
$ echo demo >file ; # dirty the tree
$ git checkout -s master ; # stash and switch to master
Uncommitted changes stashed on branch 'demo'.
$ cat file
master
$ git checkout demo ; # return to demo
Uncommitted changes were stashed for 'demo'.
To recover them now run:
git stash apply -s
$ cat file
master
$ git stash apply -s
$ cat file
demo
The new `git stash apply -s` here is defined to find the most
recent stash for the current branch (which may not be the top of
the stash!) and apply it.
If you know you want to just reapply the stash when you switch back
we could define `git checkout -a` (also unused) to automatically
execute `git stash apply -s` if a stash is available for the
destination branch.
Just thinking out loud. I probably won't code up a patch that
implements this but I don't think it would be too difficult for
someone else who wants to get their feet wet.
--
Shawn.
^ permalink raw reply
* STG, problem with pop/push of alternative versions of a patch
From: Jon Smirl @ 2007-09-15 3:49 UTC (permalink / raw)
To: Git Mailing List, Catalin Marinas
I trying to test two different versions of a patch that add files.
These patches create a new directory and add several files. When I pop
a version of the patch the directory and files and not getting
removed. This causes an error when I push the alternative version of
the patch.
jonsmirl@terra:~/mpc5200b$ stg series
+ 0001-powerpc-exports-rheap-symbol-to-modules.patch
+ 0002-powerpc-Changes-the-config-mechanism-for-rheap.patch
+ 0003-powerpc-ppc32-Update-mpc52xx_psc-structure-with-B-r.patch
+ 0004-powerpc-BestComm-core-support-for-Freescale-MPC5200.patch
+ 0005-powerpc-BestcComm-ATA-task-support.patch
+ 0006-powerpc-BestcComm-FEC-task-support.patch
+ 0007-powerpc-BestcComm-GenBD-task-support.patch
+ ignore-uImage
+ 0008-drivers-net-Add-support-for-Freescale-MPC5200-SoC-i.patch
+ 0009-sound-Add-support-for-Freescale-MPC5200-AC97-interf.patch
+ 0010-powerpc-In-rheap.c-move-the-EXPORT_SYMBOL-and-use.patch
+ 0011-powerpc-BestComm-move-the-EXPORT_SYMBOL-and-use-th.patch
+ 0012-powerpc-BestComm-ATA-task-move-the-EXPORT_SYMBOL-a.patch
+ 0013-powerpc-BestComm-FEC-task-move-the-EXPORT_SYMBOL-a.patch
+ 0014-powerpc-BestComm-GenBD-task-move-the-EXPORT_SYMBOL.patch
+ 0015-powerpc-BestComm-Replace-global-variable-bcom-by-b.patch
+ 0016-powerpc-Make-the-BestComm-driver-a-standard-of_plat.patch
+ 0017-powerpc-Fix-typo-in-BestComm-ATA-task-support-code.patch
+ 0018-powerpc-BestComm-ATA-task-microcode-insert-copyri.patch
+ 0019-powerpc-BestComm-FEC-task-microcode-insert-copyri.patch
+ 0020-powerpc-BestComm-GenBD-task-microcode-insert-copy.patch
+ 0021-powerpc-Fix-errors-in-bcom-bcom_eng-renaming.patch
+ Makefile
+ pcm030_bsp_powerpc
+ fs_jffs2_use_memcpy_fromio
+ gpio_frame
+ ppc_gpio
+ GPIO-adaption
+ mpc52xx_restart
> pcm_dts_eth_phys
- domen-fec
jonsmirl@terra:~/mpc5200b$ stg pop
0008-drivers-net-Add-support-for-Freescale-MPC5200-SoC-i.patch..
Checking for changes in the working directory ... done
Popping patches "pcm_dts_eth_phys" -
"0008-drivers-net-Add-support-for-Freescale-MPC5200-SoC-i.patch" ...
done
Now at patch "ignore-uImage"
jonsmirl@terra:~/mpc5200b$ stg series
+ 0001-powerpc-exports-rheap-symbol-to-modules.patch
+ 0002-powerpc-Changes-the-config-mechanism-for-rheap.patch
+ 0003-powerpc-ppc32-Update-mpc52xx_psc-structure-with-B-r.patch
+ 0004-powerpc-BestComm-core-support-for-Freescale-MPC5200.patch
+ 0005-powerpc-BestcComm-ATA-task-support.patch
+ 0006-powerpc-BestcComm-FEC-task-support.patch
+ 0007-powerpc-BestcComm-GenBD-task-support.patch
> ignore-uImage
- 0008-drivers-net-Add-support-for-Freescale-MPC5200-SoC-i.patch
- 0009-sound-Add-support-for-Freescale-MPC5200-AC97-interf.patch
- 0010-powerpc-In-rheap.c-move-the-EXPORT_SYMBOL-and-use.patch
- 0011-powerpc-BestComm-move-the-EXPORT_SYMBOL-and-use-th.patch
- 0012-powerpc-BestComm-ATA-task-move-the-EXPORT_SYMBOL-a.patch
- 0013-powerpc-BestComm-FEC-task-move-the-EXPORT_SYMBOL-a.patch
- 0014-powerpc-BestComm-GenBD-task-move-the-EXPORT_SYMBOL.patch
- 0015-powerpc-BestComm-Replace-global-variable-bcom-by-b.patch
- 0016-powerpc-Make-the-BestComm-driver-a-standard-of_plat.patch
- 0017-powerpc-Fix-typo-in-BestComm-ATA-task-support-code.patch
- 0018-powerpc-BestComm-ATA-task-microcode-insert-copyri.patch
- 0019-powerpc-BestComm-FEC-task-microcode-insert-copyri.patch
- 0020-powerpc-BestComm-GenBD-task-microcode-insert-copy.patch
- 0021-powerpc-Fix-errors-in-bcom-bcom_eng-renaming.patch
- Makefile
- pcm030_bsp_powerpc
- fs_jffs2_use_memcpy_fromio
- gpio_frame
- ppc_gpio
- GPIO-adaption
- mpc52xx_restart
- pcm_dts_eth_phys
- domen-fec
jonsmirl@terra:~/mpc5200b$ stg push domen-fec
Checking for changes in the working directory ... done
Pushing patch "domen-fec" ... error: drivers/net/fec_mpc52xx/Kconfig:
already exists in working directory
error: drivers/net/fec_mpc52xx/Makefile: already exists in working directory
error: drivers/net/fec_mpc52xx/fec.c: already exists in working directory
error: drivers/net/fec_mpc52xx/fec.h: already exists in working directory
error: drivers/net/fec_mpc52xx/fec_phy.c: already exists in working directory
fatal: Untracked working tree file 'drivers/net/fec_mpc52xx/Kconfig'
would be overwritten by merge.
Error: The merge failed during "push".
Use "refresh" after fixing the conflicts or revert the
operation with "push --undo".
stg push: git-merge-recursive failed with code 128
jonsmirl@terra:~/mpc5200b$ stg push --undo
Undoing push of "domen-fec" ... done
Now at patch "ignore-uImage"
jonsmirl@terra:~/mpc5200b$ stg status
? drivers/net/fec_mpc52xx/
jonsmirl@terra:~/mpc5200b$
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: Data Integrity & un-Commited Branches
From: Brian Scott Dobrovodsky @ 2007-09-15 6:24 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git
In-Reply-To: <20070915025129.GY3099@spearce.org>
> My point is just that some people actually assume that work done
> while having one branch checked out is related to that branch and
> that branch alone and that switching a branch should put that work
> on hold. Unfortunately for me some of these people at day-job have
> also just assumed Git can read their mind and forget to switch
> branches at the proper times, resulting in unrelated work mashed
> together for days straight (and criss-crossed merge to hell and back)
> before they call me and say "MAKEITWORKNOW".
> </rant>
As I have learned over the years, assumptions can be fatal. I can not
use something until I wrap my head around it and test it. Especially
for managing something in production! So far, this has been the only
problem/mis-understanding.
> It isn't unreasonable to want Git to save uncommitted work for the
> current branch and then you switch to another, ending up with a
> clean working directory when you finally get there. Today we have
> git-stash to help you with this, but I'm thinking maybe we want to
> connect git-checkout with it?
That would be great as a default action when using checkout!
+Switching branches without having to commit improves work flow.
+Fewer commits = cleaner logs.
+More Intuitive!
I am currently using git-1.5.1.6, which apparently does not have
git-stash. I will upgrade and check it out.
Cheers,
--
Brian Scott Dobrovodsky
^ permalink raw reply
* Re: Data Integrity & un-Commited Branches
From: Shawn O. Pearce @ 2007-09-15 6:32 UTC (permalink / raw)
To: Brian Scott Dobrovodsky; +Cc: git
In-Reply-To: <2a8a071a0709142324i29a863b7x8c164a589c1f1f9a@mail.gmail.com>
Brian Scott Dobrovodsky <brian@pontech.com> wrote:
> > It isn't unreasonable to want Git to save uncommitted work for the
> > current branch and then you switch to another, ending up with a
> > clean working directory when you finally get there. Today we have
> > git-stash to help you with this, but I'm thinking maybe we want to
> > connect git-checkout with it?
>
> That would be great as a default action when using checkout!
Well, a lot of "Git old timers" like the current action of keeping
the tree dirty during a switch. But maybe we could also teach `git
checkout` that a user specified configuration option can cause it
to automatically stash/unstash unless -m is supplied. Or something.
Patches are always welcome. ;-)
> +Switching branches without having to commit improves work flow.
> +Fewer commits = cleaner logs.
Well, I'm not sure that matters here. Typically Git users will make
heavy use of commit rewriting features (e.g. `git commit --amend`
or `git rebase -i`) to cleanup changes on a side branch before they
submit them to the mainline. This makes it easy to commit all of
the time and not worry about how the resulting logs will look.
Plus they can have look like they have some serious code-fu and
always write things perfectly the first time. :)
Indeed, before git-stash came about I parked changes on a branch
using the following technique:
$ git commit -a -m PARK ; # stash on "demo"
$ git checkout master ; # tree is now clean
$ git checkout demo ; # back on demo
$ git reset --soft HEAD^ ; # undo the stash
No messy history, nice neat per-branch stash. Oh, you can do that
in Git 1.3. And even earlier probably. I actually still use this
trick from time to time as I find it flowing out of my fingers far
easier than git-stash.
--
Shawn.
^ permalink raw reply
* Re: Data Integrity & un-Commited Branches
From: Junio C Hamano @ 2007-09-15 6:37 UTC (permalink / raw)
To: Brian Scott Dobrovodsky; +Cc: Shawn O. Pearce, git
In-Reply-To: <2a8a071a0709142324i29a863b7x8c164a589c1f1f9a@mail.gmail.com>
"Brian Scott Dobrovodsky" <brian@pontech.com> writes:
>> It isn't unreasonable to want Git to save uncommitted work for the
>> current branch and then you switch to another, ending up with a
>> clean working directory when you finally get there. Today we have
>> git-stash to help you with this, but I'm thinking maybe we want to
>> connect git-checkout with it?
>
> That would be great as a default action when using checkout!
I would not bet you will stay feeling that way as you gain
experience. With "git stash create" (will be in 'next' this
weekend), we could start using stashes more transparently from
other commands, but I do not think this will ever become the
default for branch switching, while I do not oppose to have such
an option.
^ permalink raw reply
* Re: Data Integrity & un-Commited Branches
From: Brian Scott Dobrovodsky @ 2007-09-15 7:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Shawn O. Pearce, git
In-Reply-To: <7vk5qs8me5.fsf@gitster.siamese.dyndns.org>
> I would not bet you will stay feeling that way as you gain
> experience. With "git stash create" (will be in 'next' this
> weekend), we could start using stashes more transparently from
> other commands, but I do not think this will ever become the
> default for branch switching, while I do not oppose to have such
> an option.
Stashing as the core default may have been over zealous..
Cheers,
--
Brian Scott Dobrovodsky
^ permalink raw reply
* [PATCH 1/5] Properly cleanup in http_cleanup so builtin-fetch does not segfault
From: Shawn O. Pearce @ 2007-09-15 7:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Junio and I both noticed that the new builtin-fetch was segfaulting
immediately on http/https/ftp style URLs (those that went through
libcurl and the commit walker). Although the builtin-fetch changes
in this area were really just minor refactorings there was one major
change made: we invoked http_init(), http_cleanup() then http_init()
again in the same process.
When we call curl_easy_cleanup() on each active_request_slot we
are telling libcurl we did not want that buffer to be used again.
Unfortunately we did not also deallocate the active_request_slot
itself nor did we NULL out active_queue_head. This lead us to
attempt to reuse these cleaned up libcurl handles when we later tried
to invoke http_init() a second time to reactivate the curl library.
The next file get operation then immediately segfaulted on most
versions of libcurl.
Properly freeing our own buffers and clearing the list causes us to
reinitialize the curl buffers again if/when we need to use libcurl
from within this same process.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
http.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/http.c b/http.c
index a95483b..87ebf7b 100644
--- a/http.c
+++ b/http.c
@@ -276,6 +276,7 @@ void http_cleanup(void)
#endif
while (slot != NULL) {
+ struct active_request_slot *next = slot->next;
#ifdef USE_CURL_MULTI
if (slot->in_use) {
curl_easy_getinfo(slot->curl,
@@ -287,8 +288,10 @@ void http_cleanup(void)
#endif
if (slot->curl != NULL)
curl_easy_cleanup(slot->curl);
- slot = slot->next;
+ free(slot);
+ slot = next;
}
+ active_queue_head = NULL;
#ifndef NO_CURL_EASY_DUPHANDLE
curl_easy_cleanup(curl_default);
@@ -300,7 +303,7 @@ void http_cleanup(void)
curl_global_cleanup();
curl_slist_free_all(pragma_header);
- pragma_header = NULL;
+ pragma_header = NULL;
}
struct active_request_slot *get_active_slot(void)
--
1.5.3.1.84.gaf82-dirty
^ permalink raw reply related
* [PATCH 2/5] Don't bother passing ref log details to walker in builtin-fetch
From: Shawn O. Pearce @ 2007-09-15 7:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
When using the walker API within builtin-fetch we don't allow
it to update refs locally; instead that action is reserved for
builtin-fetch's own main loop once the objects have actually
been downloaded.
Passing NULL here will bypass the unnecessary malloc/free of a
string buffer within the walker API. That buffer is never used
because the prior argument (the refs to update) is also NULL.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
transport.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/transport.c b/transport.c
index 0338ed4..0021190 100644
--- a/transport.c
+++ b/transport.c
@@ -26,7 +26,7 @@ static int fetch_objs_via_walker(struct transport *transport,
for (i = 0; i < nr_objs; i++)
objs[i] = xstrdup(sha1_to_hex(to_fetch[i]->old_sha1));
- if (walker_fetch(walker, nr_objs, objs, NULL, dest))
+ if (walker_fetch(walker, nr_objs, objs, NULL, NULL))
die("Fetch failed.");
for (i = 0; i < nr_objs; i++)
--
1.5.3.1.84.gaf82-dirty
^ permalink raw reply related
* [PATCH 3/5] Cleanup duplicate initialization code in transport_get
From: Shawn O. Pearce @ 2007-09-15 7:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
We always allocate and return a struct transport* right now as every
URL is considered to be a native Git transport if it is not rsync,
http/https/ftp or a bundle. So we can simplify the initialization
of a new transport object by performing one xcalloc call and filling
in only the attributes required.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
transport.c | 27 ++++++++++-----------------
1 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/transport.c b/transport.c
index 0021190..5eabe8d 100644
--- a/transport.c
+++ b/transport.c
@@ -411,27 +411,26 @@ static int is_file(const char *url)
struct transport *transport_get(struct remote *remote, const char *url,
int fetch)
{
- struct transport *ret = NULL;
+ struct transport *ret = xcalloc(1, sizeof(*ret));
+
+ ret->remote = remote;
+ ret->url = url;
+ ret->fetch = !!fetch;
+
if (!prefixcmp(url, "rsync://")) {
- ret = xmalloc(sizeof(*ret));
- ret->data = NULL;
ret->ops = &rsync_transport;
- } else if (!prefixcmp(url, "http://") || !prefixcmp(url, "https://") ||
- !prefixcmp(url, "ftp://")) {
- ret = xmalloc(sizeof(*ret));
+ } else if (!prefixcmp(url, "http://")
+ || !prefixcmp(url, "https://")
+ || !prefixcmp(url, "ftp://")) {
ret->ops = &curl_transport;
if (fetch)
ret->data = get_http_walker(url);
- else
- ret->data = NULL;
} else if (is_local(url) && is_file(url)) {
struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
- ret = xmalloc(sizeof(*ret));
ret->data = data;
ret->ops = &bundle_transport;
} else {
struct git_transport_data *data = xcalloc(1, sizeof(*data));
- ret = xcalloc(1, sizeof(*ret));
ret->data = data;
data->thin = 1;
data->uploadpack = "git-upload-pack";
@@ -443,13 +442,7 @@ struct transport *transport_get(struct remote *remote, const char *url,
data->unpacklimit = -1;
ret->ops = &git_transport;
}
- if (ret) {
- ret->remote = remote;
- ret->url = url;
- ret->remote_refs = NULL;
- ret->fetch = !!fetch;
- ret->pack_lockfile = NULL;
- }
+
return ret;
}
--
1.5.3.1.84.gaf82-dirty
^ permalink raw reply related
* [PATCH 5/5] Remove unnecessary 'fetch' argument from transport_get API
From: Shawn O. Pearce @ 2007-09-15 7:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
We don't actually need to know at the time of transport_get if the
caller wants to fetch, push, or do both on the returned object.
It is easier to just delay the initialization of the HTTP walker
until we know we will need it by providing a CURL specific fetch
function in the curl_transport that makes sure the walker instance
is initialized before use.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
builtin-fetch.c | 2 +-
builtin-push.c | 2 +-
transport.c | 23 +++++++++++++++++------
transport.h | 4 +---
4 files changed, 20 insertions(+), 11 deletions(-)
diff --git a/builtin-fetch.c b/builtin-fetch.c
index 8b0fdbe..300d563 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -496,7 +496,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
else
remote = remote_get(argv[i++]);
- transport = transport_get(remote, remote->uri[0], 1);
+ transport = transport_get(remote, remote->uri[0]);
if (verbose >= 2)
transport->verbose = 1;
if (quiet)
diff --git a/builtin-push.c b/builtin-push.c
index f496b46..7d7e826 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -59,7 +59,7 @@ static int do_push(const char *repo, int flags)
errs = 0;
for (i = 0; i < remote->uri_nr; i++) {
struct transport *transport =
- transport_get(remote, remote->uri[i], 0);
+ transport_get(remote, remote->uri[i]);
int err;
if (receivepack)
transport_set_option(transport,
diff --git a/transport.c b/transport.c
index 5eabe8d..7f94d30 100644
--- a/transport.c
+++ b/transport.c
@@ -174,6 +174,14 @@ static struct ref *get_refs_via_curl(const struct transport *transport)
return refs;
}
+static int fetch_objs_via_curl(struct transport *transport,
+ int nr_objs, struct ref **to_fetch)
+{
+ if (!transport->data)
+ transport->data = get_http_walker(transport->url);
+ return fetch_objs_via_walker(transport, nr_objs, to_fetch);
+}
+
#else
static struct ref *get_refs_via_curl(const struct transport *transport)
@@ -182,12 +190,19 @@ static struct ref *get_refs_via_curl(const struct transport *transport)
return NULL;
}
+static int fetch_objs_via_curl(struct transport *transport,
+ int nr_objs, struct ref **to_fetch)
+{
+ die("Cannot fetch from '%s' without curl ...", transport->url);
+ return -1;
+}
+
#endif
static const struct transport_ops curl_transport = {
/* set_option */ NULL,
/* get_refs_list */ get_refs_via_curl,
- /* fetch */ fetch_objs_via_walker,
+ /* fetch */ fetch_objs_via_curl,
/* push */ curl_transport_push,
/* disconnect */ disconnect_walker
};
@@ -408,14 +423,12 @@ static int is_file(const char *url)
return S_ISREG(buf.st_mode);
}
-struct transport *transport_get(struct remote *remote, const char *url,
- int fetch)
+struct transport *transport_get(struct remote *remote, const char *url)
{
struct transport *ret = xcalloc(1, sizeof(*ret));
ret->remote = remote;
ret->url = url;
- ret->fetch = !!fetch;
if (!prefixcmp(url, "rsync://")) {
ret->ops = &rsync_transport;
@@ -423,8 +436,6 @@ struct transport *transport_get(struct remote *remote, const char *url,
|| !prefixcmp(url, "https://")
|| !prefixcmp(url, "ftp://")) {
ret->ops = &curl_transport;
- if (fetch)
- ret->data = get_http_walker(url);
} else if (is_local(url) && is_file(url)) {
struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
ret->data = data;
diff --git a/transport.h b/transport.h
index f2bbdf7..6a95d66 100644
--- a/transport.h
+++ b/transport.h
@@ -6,7 +6,6 @@
struct transport {
unsigned verbose : 1;
- unsigned fetch : 1;
struct remote *remote;
const char *url;
@@ -38,8 +37,7 @@ struct transport_ops {
};
/* Returns a transport suitable for the url */
-struct transport *transport_get(struct remote *remote, const char *url,
- int fetch);
+struct transport *transport_get(struct remote *, const char *);
/* Transport options which apply to git:// and scp-style URLs */
--
1.5.3.1.84.gaf82-dirty
^ permalink raw reply related
* [PATCH 4/5] Add transport.h to LIB_H as transport.o is in LIB_OBJS
From: Shawn O. Pearce @ 2007-09-15 7:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Any changes to transport.h probably will require rebuilding a
number of object files so we should make sure it is included
in our set of headers.
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
Makefile | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/Makefile b/Makefile
index ac17510..9c7fba7 100644
--- a/Makefile
+++ b/Makefile
@@ -284,7 +284,7 @@ LIB_H = \
run-command.h strbuf.h tag.h tree.h git-compat-util.h revision.h \
tree-walk.h log-tree.h dir.h path-list.h unpack-trees.h builtin.h \
utf8.h reflog-walk.h patch-ids.h attr.h decorate.h progress.h \
- mailmap.h remote.h
+ mailmap.h remote.h transport.h
DIFF_OBJS = \
diff.o diff-lib.o diffcore-break.o diffcore-order.o \
--
1.5.3.1.84.gaf82-dirty
^ permalink raw reply related
* Re: Data Integrity & un-Commited Branches
From: Jan Hudec @ 2007-09-15 7:38 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Brian Scott Dobrovodsky, git
In-Reply-To: <20070915025129.GY3099@spearce.org>
[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]
On Fri, Sep 14, 2007 at 22:51:29 -0400, Shawn O. Pearce wrote:
> It isn't unreasonable to want Git to save uncommitted work for the
> current branch and then you switch to another, ending up with a
> clean working directory when you finally get there. Today we have
> git-stash to help you with this, but I'm thinking maybe we want to
> connect git-checkout with it?
I think it would be reasonable if it just forced you to decide about it. That
is reading the documentation, checkout only switches branches if the merge of
each modified file is trivial and only does 3-way merge if it got -m option.
It might be reasonable to requre that option for all cases, where there are
local changes and the branches don't point to the same commit and without it,
checkout should say something like:
Cannot switch branches, because the tree is modified. You can apply the
modifications to the target branch by using -m option, or commit them
before switching branches (you can undo or amend that commit later if it's
not finished yet).
The case with branches pointing to the same commit is for checkout -b,
reverting that command if you do it too early/by mistake/wanted branch
instead and for doing it with branch + checkout.
--
Jan 'Bulb' Hudec <bulb@ucw.cz>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: db/fetch-pack topic (was: What's cooking in git.git (topics))
From: Shawn O. Pearce @ 2007-09-15 7:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20070914183028.GV3099@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
> > * db/fetch-pack (Fri Sep 14 03:31:25 2007 -0400) 22 commits
> ...
> > This is Daniel's fetch-pack in C plus fixups from Shawn.
> > Unfortunately the fixups breaks t3200 ("*** glibc detected ***
> > fetch: free(): invalid pointer xxx ***"), which I haven't looked
> > into yet.
>
> Doesn't crash out on my Mac OS X system but I am getting the
> above failure on my amd64 Linux system.
OK, so in addition to the above fixups Junio mentions I have
also sent two more series today:
2 patch "fixup of the fixup" to resolve the t3200 crash
5 patch "fixup + cleanup" of http support
and now I just discovered that pushing to yourself is probably also
broken by this series:
$ git push . jc/maint:gfi-maint
updating 'refs/heads/gfi-maint' using 'refs/remotes/jc/maint'
from 05cc2ffc572f05e8aeec495a9ab9bc9609863491
to 8419d2ee9ba8b375186a5c1019df8dfbce610aba
Also local refs/heads/gfi-maint
Generating pack...
Done counting 0 objects.
Writing 0 objects...
Unpacking 0 objects...
error: Ref refs/heads/gfi-maint is at 8419d2ee9ba8b375186a5c1019df8dfbce610aba but expected 05cc2ffc572f05e8aeec495a9ab9bc9609863491
error: failed to lock refs/heads/gfi-maint
Total 0 (delta 0), reused 0 (delta 0)
ng refs/heads/gfi-maint failed to lock
error: failed to push to '.'
What's really exciting is we actually updated the ref 'gfi-maint',
even though it was "ng" and we failed to push. Yup. More work
for me to look at tomorrow. Right now I think I'm all memory
corruptioned out for *** brain detected *** Shawn(): nextword():
too tired, try sleep ***
;-)
--
Shawn.
^ permalink raw reply
* Re: Data Integrity & un-Commited Branches
From: Shawn O. Pearce @ 2007-09-15 7:51 UTC (permalink / raw)
To: Jan Hudec; +Cc: Brian Scott Dobrovodsky, git
In-Reply-To: <20070915073845.GB3782@efreet.light.src>
Jan Hudec <bulb@ucw.cz> wrote:
> On Fri, Sep 14, 2007 at 22:51:29 -0400, Shawn O. Pearce wrote:
> > It isn't unreasonable to want Git to save uncommitted work for the
> > current branch and then you switch to another, ending up with a
> > clean working directory when you finally get there. Today we have
> > git-stash to help you with this, but I'm thinking maybe we want to
> > connect git-checkout with it?
>
> I think it would be reasonable if it just forced you to decide about it. That
> is reading the documentation, checkout only switches branches if the merge of
> each modified file is trivial and only does 3-way merge if it got -m option.
>
> It might be reasonable to requre that option for all cases, where there are
> local changes and the branches don't point to the same commit and without it,
> checkout should say something like:
>
> Cannot switch branches, because the tree is modified. You can apply the
> modifications to the target branch by using -m option
The thing there is `git checkout` by default does a switch only
if the merge is really trivial. In such cases its probably sane
to carry the changes with you to the new branch/parent commit.
At worst you can safely carry them right back. Or stash them.
But -m does a three-way file merge, which isn't trivial, and can
result in conflicts.
So I know that myself and Junio both rely on the default behavior
to tell us if a switch is even a good idea right now, or if we
should stash the changes and *then* do the switch. Because if you
do the switch with -m and there are conflicts you are up a creek
with no paddle... and there's a mighty big water fall coming up
in 3 feet... 2 feet... oh crap!
Making -m the only way to switch with dirty state is not a feature.
Its a regression.
--
Shawn.
^ permalink raw reply
* Re: STG, problem with pop/push of alternative versions of a patch
From: Catalin Marinas @ 2007-09-15 8:07 UTC (permalink / raw)
To: Jon Smirl; +Cc: Git Mailing List
In-Reply-To: <9e4733910709142049k6dcec6acuf851c21ed6704287@mail.gmail.com>
On 15/09/2007, Jon Smirl <jonsmirl@gmail.com> wrote:
> I trying to test two different versions of a patch that add files.
> These patches create a new directory and add several files. When I pop
> a version of the patch the directory and files and not getting
> removed. This causes an error when I push the alternative version of
> the patch.
This shouldn't happen AFAICT (at least for the files, as GIT doesn't
care much about directories). What GIT/StGIT version are you using?
StGIT simply calls GIT to do the HEAD switch.
Could you run 'stg patches drivers/net/fec_mpc52xx/Kconfig' with the
initial series pushed? It should show which patches touch this file.
If it doesn't show any, maybe the files weren't added to any patch and
hence the error.
Thanks.
--
Catalin
^ permalink raw reply
* Re: [ANNOUNCE] tig-0.9
From: Jonas Fonseca @ 2007-09-15 9:43 UTC (permalink / raw)
To: Brian Gernhardt; +Cc: Steven Grimm, git
In-Reply-To: <719DD02E-5BA3-477E-ABBB-C385BF43EF9E@silverinsanity.com>
Brian Gernhardt <benji@silverinsanity.com> wrote Fri, Sep 14, 2007:
>
> On Sep 14, 2007, at 1:32 PM, Steven Grimm wrote:
>
> >Brian Gernhardt wrote:
> >>Complete build failure using autoconf here. Just using the
> >>Makefile like I always have works fine, but "autoconf ; ./
> >>configure" (from the git repo) fails with "configure: error: iconv
> >>() not found. Please install libiconv." This confuses me because
> >>I have /usr/lib/libiconv.dylib, and compiling with -liconv works.
> >>I fail to have the autoconf-foo to figure out what's wrong.
> >
> >Try "aclocal ; autoconf ; ./configure".
>
> Yes, remembering to regenerate the configure script would have been
> smart. Works perfectly, thank you.
Great, I will probably roll out a 0.9.1 with this fix tomorrow.
--
Jonas Fonseca
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox