* Re: git cherry to find equivalent commit IDs?
From: Avery Pennarun @ 2009-08-13 21:37 UTC (permalink / raw)
To: skillzero; +Cc: git
In-Reply-To: <2729632a0908131413w1a2efca8t31ac8cc43e1d6888@mail.gmail.com>
On Thu, Aug 13, 2009 at 9:13 PM, <skillzero@gmail.com> wrote:
> For example, commit 3642151 on branch A was a cherry pick of a commit
> 460050c on master:
>
> $ git branch -a --contains 3642151
> A
>
> $ git branch -a --contains 460050c
> * master
>
> $ git cherry -v master 3642151
> - 3642151435ce5737debc1213de46dd556475bfad1 fixed bug
>
> I assume that means an equivalent change to 3642151 is already in
> master (which it is, as commit 460050c). But I want to find out the
> commit ID on master that's equivalent to 3642151 (i.e. something that
> tells me it's 460050c).
git show 3642151 | git patch-id
You should get a line with two hashes; the first is the patchid (call
it PATCHID_FROM_ABOVE)
git log -p | git patch-id | grep PATCHID_FROM_ABOVE
This should give you a list of all commits that correspond to that patchid.
Note that if there were conflicts when applying the patch, the patchid
probably changed.
Have fun,
Avery
^ permalink raw reply
* Re: [PATCH 2/4] fast-import: define a new option command
From: Johannes Schindelin @ 2009-08-13 21:51 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Sverre Rabbelier, Git List, Junio C Hamano
In-Reply-To: <20090813175211.GQ1033@spearce.org>
Hi,
On Thu, 13 Aug 2009, Shawn O. Pearce wrote:
> Sverre Rabbelier <srabbelier@gmail.com> wrote:
> > On Thu, Aug 13, 2009 at 10:41, Shawn O. Pearce<spearce@spearce.org> wrote:
> > > Uh, no, if we have "option import-marks=..." and we can't find the
> > > file "..." and we have no --import-marks command line flag that
> > > would have overridden it, we need to abort with an error.
> >
> > Ah, then how about in option_import_marks() we only store the name of
> > the file, like in option_export_marks, and at the end, when we reach
> > the first non-option command (and we've parsed argv), we read the
> > file. That way it's only read once, and it deals with the above
> > scenario.
>
> That's better. :-)
Sorry if I spoil the party, but maybe if things get so complicated, it may
be a sign that the original version (stream overrides command line, since
it knows better) is to be preferred? After all, if hg fast-export says
that the marks should be imported from a certain file, it may be for a
_very good_ reason...
Ciao,
Dscho
^ permalink raw reply
* Re: GCC Git mirror no longer updating
From: Eric Wong @ 2009-08-13 21:51 UTC (permalink / raw)
To: Junio C Hamano
Cc: Bernie Innocenti, Jason Merrill, Daniel Berlin, Frank Ch. Eigler,
git
In-Reply-To: <4aca3dc20908130743g28a32229s194e9caa7a44fa2@mail.gmail.com>
Daniel Berlin <dberlin@dberlin.org> wrote:
> On Wed, Aug 12, 2009 at 11:37 PM, Eric Wong<normalperson@yhbt.net> wrote:
> > Bernie Innocenti <bernie@codewiz.org> wrote:
> >> El Wed, 12-08-2009 a las 09:45 -0400, Jason Merrill escribió:
> >> > On 08/12/2009 06:56 AM, Bernie Innocenti wrote:
> >> > > The git repository format should support concurrent access, but perhaps
> >> > > it only applies to git-receive-pack, not fancy operations such as
> >> > > repacking.
> >> >
> >> > The git repository format, yes, but maybe not the stuff in .git/svn. It
> >> > seems like a temporary index file was referring to an object that got
> >> > garbage collected away. Or maybe the index file was left over from the
> >> > initial import, and not there due to a collision; there don't seem to be
> >> > index files there normally.
> >>
> >> git-svn might be keeping extra information in files that the other git
> >> tools don't know about. This would explain why some objects looked
> >> like orphans and were thus culled. [cc'ing the git list to catch the
> >> attention of the git-svn maintainer(s)].
> >
> > Hi,
> >
> > As far as I can remember, no version of git svn has ever relied on
> > orphanable objects.
> >
> > Of course there are unavoidable race conditions that happen while git
> > svn is running.
> > It is never safe to run repack concurrently while git
> > svn is running (I wouldn't repack/gc simultaneously with _any_ write
> > activity on the repo).
>
> Sounds like you guys need a write lock then for certain operations.
> How do you square this with the auto-repacking the repository does (by
> default i thought it runs git gc every so often).
> We have no control over people pushing branches back at the repo, it
> may be happening when git-svn is running
Actually, I think the prune operation in git gc is the only potentially
unsafe part (and not repack). Double-checking with pruning during gc,
it seems to only expire things older than two weeks by default (when
used with gc).
So I think git svn is safe in the face of repack/gc after all.
Manually running git prune without the --expire argument isn't safe,
but we don't recommend that anyways.
> Where does it say anything about this in the docs so that people know this?
Junio: can you confirm my observations above? I think everything is
safe by default as-is. Thanks
> > git svn itself can/will run "git gc" in-between
> > revisions if needed. You can safely repack manually whenever git svn is
> > not running.
--
Eric Wong
^ permalink raw reply
* Re: [PATCH 1/5] port --ignore-unmatch to "git add"
From: Alex Riesen @ 2009-08-13 21:51 UTC (permalink / raw)
To: Luke-Jr; +Cc: Junio C Hamano, Luke Dashjr, git
In-Reply-To: <200908131540.49701.luke@dashjr.org>
On Thu, Aug 13, 2009 at 22:40, Luke-Jr<luke@dashjr.org> wrote:
> On Thursday 13 August 2009 02:36:13 pm Junio C Hamano wrote:
>> Now the obligatory design level question is behind us, let's take a brief
>> look at the codde.
>>
>> > +static int ignore_unmatch = 0;
>>
>> Drop " = 0" and let the language initialize this to zero.
>
> Does C define a default initialisation of zero? My understanding was that
> uninitialised variables are always undefined until explicitly assigned a
> value.
Yes. For statics.
^ permalink raw reply
* Re: msysGit and SCons: broken?
From: Johannes Schindelin @ 2009-08-13 21:54 UTC (permalink / raw)
To: Dirk Süsserott; +Cc: Git Mailing List
In-Reply-To: <4A846A5A.4000000@dirk.my1.cc>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1062 bytes --]
Hi,
On Thu, 13 Aug 2009, Dirk Süsserott wrote:
> Am 06.08.2009 21:45 schrieb Dirk Süsserott:
> > Am 04.08.2009 00:13 schrieb Johannes Schindelin:
> > > On Mon, 3 Aug 2009, Dirk Süsserott wrote:
> > >
> > > How does your SCons call relate to Git? Do you call it from the Git Bash?
> > > Do you call it from cmd.exe directly? Is Git/bash in your PATH?
> >
> > I used to call SCons from Git-bash and it worked. After Git's upgrade (or
> > some other unknown change) I did the same and it didn't work from Git-bash,
> > but it still worked from cmd.exe. Git-bash ist not in my PATH when I run
> > cmd.exe.
>
> If someone had the same or a similar problem: I tracked it down and
> found a solution. The problem was that I tried to run a Windows program
> from git-bash. The Windows program then faces the bash's $PATH with a
> different separator (':' vs. ';') and a different root directory ('/c/'
> vs. 'C:/').
I cannot believe that. I actually run a Windows program very often, from
Git Bash, and PATH is correctly converted all the time.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 5/8] Add a config option for remotes to specify a foreign vcs
From: Jakub Narebski @ 2009-08-13 22:00 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Johannes Schindelin, Bert Wesarg, Daniel Barkalow, git,
Brian Gernhardt
In-Reply-To: <7vocqkydum.fsf@alter.siamese.dyndns.org>
On Wed, 12 August 2009, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
>> Jeff King <peff@peff.net> writes:
>>
>>> 1. Is there some other syntax that _doesn't_ have this breakage
>>> but that similarly helps the "vast majority of Git users".
>>
>> Well, proposed possible syntax was:
>> 1. <vcs>:<repository location>
>> ...
>> 2. <vcs>::<repository location>
>> ...
>> 3. <vcs>+<repository location>
>>
>> e.g.
>>
>> svn+http://svn.example.com/project
>>
>> but
>>
>> http+svn://svn.example.com/project
>> svn+path/to/repo
>
> I do not think these are valid examples to demonstrate that 3 is bad.
>
> We do not have (and we will not create) "http+svn://" native transport, so
> the former can only mean "Feed 'svn://svn.example.com/project' to the vcs
> helper whose name is 'http'". Similarly I do not see any way to read the
> latter other than "Feed 'path/to/repo' to 'svn' vcs helper".
And not "Use 'svn+path/to/repo' as local filesystem path to repository?
On the other hand you can always use here './svn+path/to/repo'
> Double-colon (your 2) is also workable. It probably is slightly better
> than plus because it does not have to grandfather "git+ssh" and "ssh+git"
> and that would be beneficial for requiring less complexity in both code
> (i.e. special case logic) and more importantly in mental burden to the end
> users (i.e. '::' would stand out more than '+' and clearly different from
> traditional git URLs in all cases).
>
> As Jeff said (your 1.), a single colon ':' has a rather bad ambiguity
> between <vcs> and hostname part in the existing scp-style repository
> naming.
Also double colon is better for scp-like repository location, as e.g.
svn+example.com:path/to/repo
might be 'path/to/repo' on 'svn+example.com' host; there is no "escape"
mechanism like for './svn+path/to/repo' relative path. On the other
hand unescaped ':' cannot be present in hostname, therefore the following
is unambiguous:
svn::example.com:path/to/repo
But on another hand svn+http://svn.example.com/project/trunk/ looks IMVHO
better that svn::http://svn.example.com/project/trunk/
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH 2/4] fast-import: define a new option command
From: Sverre Rabbelier @ 2009-08-13 22:01 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Shawn O. Pearce, Git List, Junio C Hamano
In-Reply-To: <alpine.DEB.1.00.0908132350190.8306@pacific.mpi-cbg.de>
Heya,
On Thu, Aug 13, 2009 at 14:51, Johannes
Schindelin<Johannes.Schindelin@gmx.de> wrote:
> Sorry if I spoil the party, but maybe if things get so complicated, it may
> be a sign that the original version (stream overrides command line, since
> it knows better) is to be preferred? After all, if hg fast-export says
> that the marks should be imported from a certain file, it may be for a
> _very good_ reason...
Yes, and that should Just Work (which it does). Also, I'm not sure how
often one would output a stream on one computer, then move it to
another and import it there, but I'll methinks Shawn brought it up for
a reason ;). However, I do think it's better design to only store the
name of the import file and then do the actual import later on (to
prevent double imports).
I don't have a preference either way (both patches are already written
after all). Shawn?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH 2/4] fast-import: define a new option command
From: Shawn O. Pearce @ 2009-08-13 22:12 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Johannes Schindelin, Git List, Junio C Hamano
In-Reply-To: <fabb9a1e0908131501o73807f81mb230530c284ad123@mail.gmail.com>
Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Thu, Aug 13, 2009 at 14:51, Johannes
> Schindelin<Johannes.Schindelin@gmx.de> wrote:
> > Sorry if I spoil the party, but maybe if things get so complicated, it may
> > be a sign that the original version (stream overrides command line, since
> > it knows better) is to be preferred? ?After all, if hg fast-export says
> > that the marks should be imported from a certain file, it may be for a
> > _very good_ reason...
>
> Yes, and that should Just Work (which it does). Also, I'm not sure how
> often one would output a stream on one computer, then move it to
> another and import it there, but I'll methinks Shawn brought it up for
> a reason ;). However, I do think it's better design to only store the
> name of the import file and then do the actual import later on (to
> prevent double imports).
>
> I don't have a preference either way (both patches are already written
> after all). Shawn?
No, I don't have a really good reason for the command line overrides
the file other than this simple rule:
If the file is likely to be several hundred MiB, or bigger; thou
shall never try to open it with vi, *especially* vi on a Solaris
system, as at least one line is likely to be too long.
If the file header contains paths to other files, it is likely
one will want to modify that header sometime, because you moved
the file between systems.
Given the size of the file above, you can't just fix it with vi.
Lacking a tool that *can* do this edit safely (and Dscho's simple
sed wasn't enough, as I already pointed out, oh and Solaris sed
also fails on long lines), we *should* be able to override this on
the command line, *especially* since we already have the command
line option standardized!
What is this, gang up on Shawn's words-of-wisdom week? Both this
thread and my intern this week have been argueing with me about
what seem to me to be fairly trivial things. Maybe I just need to
take vacation. Good thing I have one coming in 5 weeks.
I say use the version where we store the values (e.g. file names)
during option parsing, and then actually apply those saved values
just before the first non-option command. Which I think only has
an impact on the import-marks option, the rest are all just simple
variable updates whose values aren't read until after the first
non-option command anyway.
--
Shawn.
^ permalink raw reply
* Re: [PATCH 2/4] fast-import: define a new option command
From: Sverre Rabbelier @ 2009-08-13 22:17 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Johannes Schindelin, Git List, Junio C Hamano
In-Reply-To: <20090813221213.GR1033@spearce.org>
Heya,
On Thu, Aug 13, 2009 at 15:12, Shawn O. Pearce<spearce@spearce.org> wrote:
> No, I don't have a really good reason for the command line overrides
> the file other than this simple rule:
Nice anecdote :P.
> What is this, gang up on Shawn's words-of-wisdom week? Both this
> thread and my intern this week have been argueing with me about
> what seem to me to be fairly trivial things. Maybe I just need to
> take vacation. Good thing I have one coming in 5 weeks.
Didn't you get the memo, you could be crashing an airplane [0] like that ;).
> I say use the version where we store the values (e.g. file names)
> during option parsing, and then actually apply those saved values
> just before the first non-option command. Which I think only has
> an impact on the import-marks option, the rest are all just simple
> variable updates whose values aren't read until after the first
> non-option command anyway.
Ok, that's v3, speaking of which, does it look good enough for inclusion?
[0] http://www.thehackerchickblog.com/2009/05/plane-crashes-software-failures-and.html
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH] Work around performance bug in subprocess.Popen.communicate()
From: Catalin Marinas @ 2009-08-13 22:18 UTC (permalink / raw)
To: Karl Wiberg; +Cc: mandolaerik, git
In-Reply-To: <20090804085125.GB27452@lux.e.vtech>
2009/8/4 Karl Wiberg <kha@virtutech.com>:
> On 2009-07-31 12:27:53 +0100, Catalin Marinas wrote:
>
>> But can this not lead to a deadlock if the __indata is big? The
>> stdout of the created process is only processed once the whole
>> __indata is written. I thought communicate() was created to avoid
>> this issue.
>
> I don't think there's a problem. write() isn't supposed to have a
> limit on the amount of data it will accept in one call, as far as I'm
> aware. Plus, it works just fine with Erik's test case---which in my
> case was about 7 MB. If it can handle 7 MB, I doubt there's a limit
> we'll hit anytime soon.
write() itself doesn't have a limit, it's mainly what the application
receiving the data can handle. In the Git case, I think it takes all
the input as it isn't a filtering tool (things may be different with
tools like sed etc.).
> Oh, and we still call communicate()---we just don't pass it any
> additional bytes to write to stdin.
Yes, but if write() is blocked, communicate() won't be called.
Since we are only using Git, I'll merge this patch (and maybe add a comment).
--
Catalin
^ permalink raw reply
* Re: [PATCH] Add 'stg prev' and 'stg next'
From: Catalin Marinas @ 2009-08-13 22:30 UTC (permalink / raw)
To: Hannes Eder; +Cc: git
In-Reply-To: <20090807104503.15805.95504.stgit@jazzy.zrh.corp.google.com>
2009/8/7 Hannes Eder <heder@google.com>:
> These commands are related to 'stg top'. They print the patch below
> resp. above the topmost patch, given that they exist.
I applied it. Thanks.
--
Catalin
^ permalink raw reply
* Re: rebase-with-history -- a technique for rebasing without trashing your repo history
From: Michael Haggerty @ 2009-08-13 22:39 UTC (permalink / raw)
To: Björn Steinbrink; +Cc: Git Mailing List
In-Reply-To: <20090813161256.GA8292@atjola.homenet>
Björn Steinbrink wrote:
> On 2009.08.13 14:46:07 +0200, Michael Haggerty wrote:
>> (Please note that this technique only works for the typical "tracking
>> upstream" type of rebase; it doesn't help with rebases whose goals are
>> changing the order of commits, moving only part of a branch, rewriting
>> commits, etc.)
>
> Hm, so that pretty much doesn't work at all for creating a clean patch
> series, which usually involves rewriting commits, squasing bug fixes
> into the original commits that introduced the bug etc.
Now that you mention it, there are some other uses of rebase whose
history could be recorded correctly, or at least better, in the DAG. I
am not ready to advocate any of these changes, but I think they are
worth discussing.
A squash of two adjacent commits currently transforms this:
A---B1---B2---C
to this
A---B12---C'
(C' has the same contents and commit message as C but a different
history and therefore a different SHA1.)
But B12 includes both B1 and B2, so the correct history is this:
A---B1---B2----C
\ \ \
---------B12---C'
The fact that B12 has B1 and B2 as ancestors tells git that it
incorporates both of their changes, which correctly describes reality.
Splitting a commit (not really an elementary rebase operation, but
achievable with "edit") transforms this:
A---B12---C
to this:
A---B1---B2---C'
It is not possible to represent this correctly in the DAG, because there
is no way to express that "B1" includes part of "B12" as a parent. But
the following would be more accurate than discarding all history:
A---B12---C
\ \ \
B1---B2---C'
It would of course be difficult for the user-interface layer to be
confident that the changes in B1 and B2 are really equivalent to B12
unless the content of B12 and B2 are identical.
Inserting a new commit into the history (for example as part of
reordering later commits) transforms this:
A---C
to this:
A---B---C'
In this case C' includes everything that is in C, so the correct history is
A---C
\ \
B---C'
However, deleting a commit, which transforms this:
A---B---C
to this:
A---C'
cannot be represented in the DAG, because there is no way to express
that C' includes the changes from C without also implying that it
includes the changes from B.
Rewriting a single commit, under the assumption that the rewritten
commit is the logical equivalent of the original, transforms this:
A---B1---C
to this:
A---B2---C'
where B2 is a hand-rewritten version of B1, and C' is the version of C
produced by the rebase. In this case, the history could be recorded as:
A---B1---C
\ \ \
----B2---C'
but again, it is impossible for the user-interface layer to ascertain
that B2 is equivalent to B1 without help from the user.
All of this extra history would currently create far more clutter than
it is worth, but if there were a way to suppress the display of rebased
commits (as discussed in the third article I quoted), then the extra
information would be there to help git without overwhelming users.
> And even for just continously forward porting a series of commits, a
> common case might be that upstream applied some patches, but not all.
> Can you deal with that?
>
> Example:
>
> A---B---C (upstream)
> \
> H---I---J---K (yours)
>
> Upstream takes some changes:
>
> A---B---C---I'--K'--D (upstream)
> \
> H---I---J---K (yours)
>
> rebase leads to:
>
> A---B---C---I'--K'--D (upstream)
> \
> H'--J' (yours)
>
> What would your approach generate in that case?
There *is no way* to represent this history in a DAG, and therefore the
history of this operation will necessarily be lost. (Well, of course it
could be recorded in metadata supplemental to the DAG, but since the
history would not affect future merges it would be pointless.) The
problem is that there is no way to claim that I' is derived from I
without also implying that I' includes the change in H (which it
doesn't). I discuss this sort of thing in another article [1].
>> For more information, please see the full articles: [...]
>
> In this one you have two DAGs:
> (I fixed the second one to also have the merge commit in "subsystem"
> instead of "topic", so they only differ WRT to the rebased stuff)
>
> A)
> m---N---m---m---m---m---m---M (master)
> \ \
> o---o---O---o---o o'--o'--o'--o'--o'--S (subsystem)
> \ /
> *---*---*-..........-*--T (topic)
>
>
> B)
> m---N---m---m---m---m---m---M (master)
> \ \
> \ o'--o'--o'--o'--o'----------S (subsystem)
> \ / / / / / /
> --------------------o---o---O---o---o---*---*---T (topic)
>
>
> And you say that the former creates problems when you want to merge
> again. How so?
As you very clearly showed (thanks!), the merge problems that I claimed
only occur in some obscure edge cases.
What I *should* have emphasized is that the merge S itself is much more
prone to conflicts in case A) (with merge base N) than in case B) (with
the last "o" as merge base). That is the first advantage of
rebase-with-history.
And please note that I really advocate C), not B):
C)
m---N---m---m---m---m---m---M master
\ \
\ o'--o'--o'--o'--o' subsystem
\ / / / / / \
--------------------o---o---O---o---o \
\ \
\ *'--*'--*' topic
\ / / /
*---*---*
where the topic branch is not merged into the subsystem branch but
rather rebased-with-history. C) has the significant advantage over A)
or B) that the topic branch can be converted to a series of patches (the
*' patches) that apply cleanly to the rebased subsystem branch and can
therefore be submitted upstream. In the case of A) or B), the only
available patch that applies cleanly to the rebased subsystem branch is
S, which is a single commit that squashes together the entire topic
branch and is therefore difficult to review.
So rebasing in a public repository makes it difficult for downstream
developers to apply their work to the rebased branch (because they have
to repeat the conflict resolution that was done in the upstream rebase),
and merging in a topic branch makes it more difficult to create an
easily-reviewable patch series. rebase-with-history has neither of
these problems.
Michael
[1] "Git, Mercurial, and Bazaar—simplicity through inflexibility",
http://softwareswirl.blogspot.com/2009/08/git-mercurial-and-bazaarsimplicity.html
^ permalink raw reply
* Re: git cherry to find equivalent commit IDs?
From: skillzero @ 2009-08-13 23:05 UTC (permalink / raw)
To: Avery Pennarun; +Cc: git
In-Reply-To: <32541b130908131437g7cb14a1eh6cd2e31a57f105d5@mail.gmail.com>
On Thu, Aug 13, 2009 at 2:37 PM, Avery Pennarun<apenwarr@gmail.com> wrote:
> On Thu, Aug 13, 2009 at 9:13 PM, <skillzero@gmail.com> wrote:
>> For example, commit 3642151 on branch A was a cherry pick of a commit
>> 460050c on master:
>>
>> $ git branch -a --contains 3642151
>> A
>>
>> $ git branch -a --contains 460050c
>> * master
>>
>> $ git cherry -v master 3642151
>> - 3642151435ce5737debc1213de46dd556475bfad1 fixed bug
>>
>> I assume that means an equivalent change to 3642151 is already in
>> master (which it is, as commit 460050c). But I want to find out the
>> commit ID on master that's equivalent to 3642151 (i.e. something that
>> tells me it's 460050c).
>
> git show 3642151 | git patch-id
>
> You should get a line with two hashes; the first is the patchid (call
> it PATCHID_FROM_ABOVE)
>
> git log -p | git patch-id | grep PATCHID_FROM_ABOVE
>
> This should give you a list of all commits that correspond to that patchid.
That worked perfectly. Thanks.
^ permalink raw reply
* Re: rebase-with-history -- a technique for rebasing without trashing your repo history
From: Björn Steinbrink @ 2009-08-13 23:30 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Git Mailing List
In-Reply-To: <4A849634.1020609@alum.mit.edu>
On 2009.08.14 00:39:48 +0200, Michael Haggerty wrote:
> Björn Steinbrink wrote:
> > On 2009.08.13 14:46:07 +0200, Michael Haggerty wrote:
> > And even for just continously forward porting a series of commits, a
> > common case might be that upstream applied some patches, but not all.
> > Can you deal with that?
> >
> > Example:
> >
> > A---B---C (upstream)
> > \
> > H---I---J---K (yours)
> >
> > Upstream takes some changes:
> >
> > A---B---C---I'--K'--D (upstream)
> > \
> > H---I---J---K (yours)
> >
> > rebase leads to:
> >
> > A---B---C---I'--K'--D (upstream)
> > \
> > H'--J' (yours)
> >
> > What would your approach generate in that case?
>
> There *is no way* to represent this history in a DAG, and therefore the
> history of this operation will necessarily be lost. (Well, of course it
> could be recorded in metadata supplemental to the DAG, but since the
> history would not affect future merges it would be pointless.) The
> problem is that there is no way to claim that I' is derived from I
> without also implying that I' includes the change in H (which it
> doesn't). I discuss this sort of thing in another article [1].
Well, I' isn't even interesting here. I' is _upstream's_ commit, likely
created from an email. Upstream didn't have "I" when I' was created to
begin with. The interesting commits are H' and J', which were actually
rebased. And the situation is even "worse" than for I' or K'. H' does
include H, I, and K, but not J and J' contains H, I, J and K.
So possibly you'd have:
A---B---C---I'--K'--D (upstream)
\ \
\ --------H'--J'
\ / /
H---I---J---K----
Which only ignores that K is "contained" in H'.
But you only get to know that I is in H' (and that K is in J') _after_
H' (or J') have been created. So you'd need a preprocessing run.
The non-preprocessing result would be something like:
A---B---C---I'--K'--D (upstream)
\ \
\ -------------H'--J'
\/ /
H---I---J--------
\
K
But that's obviously total crap.
> >> For more information, please see the full articles: [...]
> >
> > In this one you have two DAGs:
> > (I fixed the second one to also have the merge commit in "subsystem"
> > instead of "topic", so they only differ WRT to the rebased stuff)
> >
> > A)
> > m---N---m---m---m---m---m---M (master)
> > \ \
> > o---o---O---o---o o'--o'--o'--o'--o'--S (subsystem)
> > \ /
> > *---*---*-..........-*--T (topic)
> >
> >
> > B)
> > m---N---m---m---m---m---m---M (master)
> > \ \
> > \ o'--o'--o'--o'--o'----------S (subsystem)
> > \ / / / / / /
> > --------------------o---o---O---o---o---*---*---T (topic)
> >
> >
> > And you say that the former creates problems when you want to merge
> > again. How so?
>
> As you very clearly showed (thanks!), the merge problems that I claimed
> only occur in some obscure edge cases.
>
> What I *should* have emphasized is that the merge S itself is much more
> prone to conflicts in case A) (with merge base N) than in case B) (with
> the last "o" as merge base). That is the first advantage of
> rebase-with-history.
Yeah, but as I said, actually, topic should have been rebased, not
merged, and then that ends up the same, as you'd do:
git rebase --onto subsystem $last_o topic
Taking the last o commit as upstream.
> And please note that I really advocate C), not B):
>
> C)
> m---N---m---m---m---m---m---M master
> \ \
> \ o'--o'--o'--o'--o' subsystem
> \ / / / / / \
> --------------------o---o---O---o---o \
> \ \
> \ *'--*'--*' topic
> \ / / /
> *---*---*
>
Fine, but this should then be compared to the result from the above
rebase command, which is:
D)
m--...--m (master)
\
o'-...-o' (subsystem)
\
*'-...-*' (topic)
> where the topic branch is not merged into the subsystem branch but
> rather rebased-with-history. C) has the significant advantage over A)
> or B) that the topic branch can be converted to a series of patches (the
> *' patches) that apply cleanly to the rebased subsystem branch and can
> therefore be submitted upstream. In the case of A) or B), the only
> available patch that applies cleanly to the rebased subsystem branch is
> S, which is a single commit that squashes together the entire topic
> branch and is therefore difficult to review.
Same for D)
> So rebasing in a public repository makes it difficult for downstream
> developers to apply their work to the rebased branch (because they have
> to repeat the conflict resolution that was done in the upstream rebase),
There's no difference between C) and D) there, except for the fact that
D) requires you to use --onto, because that needs to differ from
<upstream>.
Let's take the pre-topic-rebase history:
m---m---m (master)
\ \
\ o'--o'--O' (subsystem)
\
o---o---O---*---*---* (topic)
Doing a plain "git rebase subsystem topic" would of course also try to
rebase the "o" commits, so that problematic. Instead, you do:
git rebase --onto subsystem O topic
That turns O..topic (the * commits) into patches, and applies them on
top of O'. So the "o" commits aren't to be rebased.
And that's exactly what your rebase-with-history would do as well. Just
that O is naturally a common ancestor of subsystem and topic, and so
just using "git rebase-w-h subsystem topic" would be enough. Conflicts
etc. should be 100% the same.
If you know that your upstream is going to rebase/rewrite history, you
can tag (or otherwise mark) the current branching point of your branch,
so you can easily specify it for the --onto rebase. IOW: This is
primarily a social problem (tell your downstream that you rebase this or
that branch), but having built-in support to store the branching point
for rebasing _might_ be worth a thought.
> and merging in a topic branch makes it more difficult to create an
> easily-reviewable patch series. rebase-with-history has neither of
> these problems.
Sure, merging is a no-go if you submit patches by email (or other,
similar means). But you compared that to an "enhanced" rebase approach,
instead of comparing your rebase approach to the currently available
one.
So, as I see it, your approach does:
* Save the need to use --onto, allowing to just specify <upstream>, as
if <upstream> was not rewritten.
* Allows to keep older versions of commits more easily accessible for
inspection, e.g. creating interdiffs.
The latter is (to me) of limited use, and the former could be done by
tracking the branching point, not sure how well it work out, but maybe
worth investigating.
Björn
^ permalink raw reply
* Re: rebase-with-history -- a technique for rebasing without trashing your repo history
From: Sitaram Chamarty @ 2009-08-14 3:17 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Björn Steinbrink, Git Mailing List
In-Reply-To: <4A849634.1020609@alum.mit.edu>
Hi,
I'm one of those wannabe experts who thinks he knows enough about git
to teach people in his workplace but obviously pales in this group,
but with that caveat, let me say:
2009/8/14 Michael Haggerty <mhagger@alum.mit.edu>:
> Now that you mention it, there are some other uses of rebase whose
> history could be recorded correctly, or at least better, in the DAG. I
> am not ready to advocate any of these changes, but I think they are
I see you've made your own caveat :-)
> worth discussing.
[snip]
> A---B1---B2----C
> \ \ \
> ---------B12---C'
> A---B12---C
> \ \ \
> B1---B2---C'
[snip]
> A---C
> \ \
> B---C'
[etc etc... many such snipped]
To me, the ability to *forget* the mistakes I made (for whatever
definition of "mistake" you wish) as long as it's private to my repo,
is one of the main attractions of git. I'm one of those guys who
saves early, and saves often, when editing files. This translates to
commit early, commit often, in the git world.
I see no earthly reason why I would ever *want* those commits
preserved, so I hope that, if this sort of thing ever gets into the
code, it is definitely *not* the default :-) It is not sufficient for
me that the GUI knows how to suppress their display, it is necessary
that they *disappear completely*.
And that reminds me. You often hear people on #git ask how to get rid
of some files (maybe containing passwords etc) that inadvertently got
into the repo, and the answer, a lot of the time, is filter-branch,
because the "bad" commit is pretty old. I suspect that for every
person who asks that question on the list because he already pushed,
there are 4 who discovered such an error much earlier, (when the file
went into only a couple of commits at the top maybe), did a rebase -i
with "edit" or whatever, and got rid of the evidence, err I mean
password :-) If this sort of thing were to be the default, they'd
have to use a filter-branch even for such simple cases.
Finally, speaking as someone who teaches git, this adds enormous
complexity to the basic concepts. Complexity is good when the
benefits are obvious, but to me they are not obvious [see *my* caveat
at the top before you react to this statement]
^ permalink raw reply
* [RFC PATCH 0/2] add a --delete option to git push
From: Sverre Rabbelier @ 2009-08-14 5:05 UTC (permalink / raw)
To: Jakub Narebski, Junio C Hamano, Git List
This is in reply to a brief conversation I had with jnareb on #git
about the blogpost "5 things git could learn from hg", or something
like that (can't find it right now).
Comments most welcome.
Sverre Rabbelier (2):
add a --delete option to git push
test that git push --delete deletes the remote ref
Documentation/git-push.txt | 8 +++++++-
builtin-push.c | 10 +++++++++-
t/t5516-fetch-push.sh | 6 ++++++
3 files changed, 22 insertions(+), 2 deletions(-)
^ permalink raw reply
* [RFC PATCH 1/2] add a --delete option to git push
From: Sverre Rabbelier @ 2009-08-14 5:05 UTC (permalink / raw)
To: Jakub Narebski, Junio C Hamano, Git List; +Cc: Sverre Rabbelier
In-Reply-To: <1250226349-20397-1-git-send-email-srabbelier@gmail.com>
Those new to git have a hard time learning how to delete a remote
ref. This makes it easier for them to find out how by providing a
flag to git push.
Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---
Currently `git push --delete master:master` results in a somewhat
cryptic error message. It seems unlikely however, that those new
to git would use the 'old:new' notation, so I haven't bothered
guarding against it and settled for documenting it.
Documentation/git-push.txt | 8 +++++++-
builtin-push.c | 10 +++++++++-
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 58d2bd5..1ecc6ca 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -10,7 +10,7 @@ SYNOPSIS
--------
[verse]
'git push' [--all | --mirror | --tags] [--dry-run] [--receive-pack=<git-receive-pack>]
- [--repo=<repository>] [-f | --force] [-v | --verbose]
+ [--repo=<repository>] [-f | --force] [-v | --verbose] [-d | --delete]
[<repository> <refspec>...]
DESCRIPTION
@@ -137,6 +137,12 @@ useful if you write an alias or script around 'git-push'.
--verbose::
Run verbosely.
+-d::
+--delete::
+ Delete the specified refs. Prefixes all refs with ':' to tell the
+ push machinery to delete the specified ref. As such, the refs
+ that are to be deleted should not contain a ':' specifier.
+
include::urls-remotes.txt[]
OUTPUT
diff --git a/builtin-push.c b/builtin-push.c
index 67f6d96..b954235 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -10,11 +10,12 @@
#include "parse-options.h"
static const char * const push_usage[] = {
- "git push [--all | --mirror] [--dry-run] [--porcelain] [--tags] [--receive-pack=<git-receive-pack>] [--repo=<repository>] [-f | --force] [-v] [<repository> <refspec>...]",
+ "git push [--all | --mirror] [--dry-run] [--porcelain] [--tags] [--receive-pack=<git-receive-pack>] [--repo=<repository>] [-f | --force] [-v] [-d | --delete] [<repository> <refspec>...]",
NULL,
};
static int thin;
+static int push_delete;
static const char *receivepack;
static const char **refspec;
@@ -44,6 +45,12 @@ static void set_refspecs(const char **refs, int nr)
strcat(tag, refs[i]);
ref = tag;
}
+ if (push_delete) {
+ struct strbuf deleted = STRBUF_INIT;
+ strbuf_addstr(&deleted, ":");
+ strbuf_addstr(&deleted, refs[i]);
+ ref = strbuf_detach(&deleted, NULL);
+ }
add_refspec(ref);
}
}
@@ -188,6 +195,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
OPT_BOOLEAN( 0 , "thin", &thin, "use thin pack"),
OPT_STRING( 0 , "receive-pack", &receivepack, "receive-pack", "receive pack program"),
OPT_STRING( 0 , "exec", &receivepack, "receive-pack", "receive pack program"),
+ OPT_BOOLEAN('d', "delete", &push_delete, "delete ref"),
OPT_END()
};
--
1.6.4.16.g72c66.dirty
^ permalink raw reply related
* [RFC PATCH 2/2] test that git push --delete deletes the remote ref
From: Sverre Rabbelier @ 2009-08-14 5:05 UTC (permalink / raw)
To: Jakub Narebski, Junio C Hamano, Git List; +Cc: Sverre Rabbelier
In-Reply-To: <1250226349-20397-2-git-send-email-srabbelier@gmail.com>
Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---
Not a lot of tests yet, but I suspect reviewers will ask for more
and/or new features will pop up that need testing, hence a
seperate patch with tests.
t/t5516-fetch-push.sh | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 2d2633f..64da9a6 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -586,4 +586,10 @@ test_expect_success 'push with branches containing #' '
git checkout master
'
+test_expect_success 'allow deleting a remote ref with --delete' '
+ mk_test heads/master &&
+ git push --delete testrepo refs/heads/master &&
+ (cd testrepo && test_must_fail git rev-parse --verify refs/heads/master)
+'
+
test_done
--
1.6.4.16.g72c66.dirty
^ permalink raw reply related
* Re: [RFC PATCH 1/2] add a --delete option to git push
From: Jeff King @ 2009-08-14 5:21 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Jakub Narebski, Junio C Hamano, Git List
In-Reply-To: <1250226349-20397-2-git-send-email-srabbelier@gmail.com>
On Thu, Aug 13, 2009 at 10:05:48PM -0700, Sverre Rabbelier wrote:
> Those new to git have a hard time learning how to delete a remote
> ref. This makes it easier for them to find out how by providing a
> flag to git push.
Should this also impact refs read from the config? E.g., if I do "git
push --delete" will it try to impact matching refs (which is almost
certainly a mistake), or refs I have in my remote.$X.push (which is
probably also a mistake)?
>From reading your patch, it looks like it just touches the command-line.
I think that's the right thing to do, but I think it makes sense to
think half a second to make sure.
And with the way you have it, "git push --delete" will silently ignore
the --delete and push configured refspecs. Probably it should say
"--delete is useless without refspecs on the command line".
> Currently `git push --delete master:master` results in a somewhat
> cryptic error message. It seems unlikely however, that those new
> to git would use the 'old:new' notation, so I haven't bothered
> guarding against it and settled for documenting it.
It seems like it would be simple enough to just check whether the
refspec contains a colon; if so, silently leave it alone. That could
also protect configured refspecs, as mentioned above, but I wouldn't
rule out somebody have a single-name refspec in their config (in fact, I
think "remote.$X.push = HEAD" is reasonable -- should that delete the
HEAD on "git push --delete"?).
> +--delete::
> + Delete the specified refs. Prefixes all refs with ':' to tell the
> + push machinery to delete the specified ref. As such, the refs
> + that are to be deleted should not contain a ':' specifier.
> +
This impacts _all_ refspecs. Remember that we can have multiple refspecs
on the command-line. So I can "move" a ref remotely with:
git push :old-name old-name:new-name
but I can't do:
git push --delete old-name old-name:new-name
Ignoring colon-less refspecs would make that work, but would not allow
the similar:
# branch has already been renamed to new-name locally
git push --delete old-name new-name
So maybe it would make more sense for it to be "--delete <ref>" and
impact only a single ref. The simple case of "git push --delete foo"
would remain unchanged.
The counter-argument is that "--delete" does not necessarily need to be
as powerful as the ":ref" syntax, but I don't see the downside in making
it so.
-Peff
^ permalink raw reply
* Re: [PATCH] Fix "unpack-objects --strict"
From: Martin Koegler @ 2009-08-14 6:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Frank Lichtenheld, git
In-Reply-To: <7vd46zbjae.fsf@alter.siamese.dyndns.org>
On Thu, Aug 13, 2009 at 12:33:45PM -0700, Junio C Hamano wrote:
> diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c
> index 557148a..109b7c8 100644
> --- a/builtin-unpack-objects.c
> +++ b/builtin-unpack-objects.c
What about this check:
> @@ -184,7 +184,7 @@ static int check_object(struct object *obj, int type, void *data)
> if (!obj)
> return 0;
This is neccessary to skip already written objects (eg. blobs,
obj_list[i].obj == NULL). The return code is not important in this
case.
I'm not sure, if fsck_walk can call check_object with obj == NULL
under some (rare) conditions. If yes, the return code should be
changed to 1.
> We probably should write everything as they become unpackable (i.e. as
> their delta bases becomes available) while keeping track of object names
> (but not data) of structured objects that we received, and running only
> one level of reachability check on them at the end. That would certainly
> reduce the memory consumption and may simplify the complexity of the code
> at the same time.
This would defeat the whole idea of the this check: If the
precondition (fsck returns OK for a repository) is met, unpack-objects
(and index-pack) with --strict should gurantee, that this is still
true after receiving the objects (even if somebody intentionally tries
to corrupt the repository).
As we assume, that all objects (and all their ancestors) already
present in the repository are OK, we only have to check new objects
and verify, that linked objects in the repository are of the correct
type.
As soon as we start to write objects without all linked objects
already present in the repository, the repository can get inconsistant.
Lets assume, unpack-objects/receive-pack is changed according to your proposal:
* writeout all objects, as received
* checking reachability of new objects
* update refs, if everything is OK
Then a corruption HOWTO would be:
To introduce a object with one of its linked objects missing, left it
out of the pack and push it into the repository. unpack-objects will
unpack all objects and fail updating the ref (but leave all objects in
the repository). As second step, simply send a ref update request,
which should succed, as the object is present in the repository.
A variant: set the SHA1 of the missing object to the SHA1 of a object
of another type. In the second step, you can sent this object to
unpack-objects, which will unpack it, as it does not know, that the
object is already referenced in the repository.
Deleting objects in the case of an error is also not an option, as a
parallel push operation could have already used the object.
mfg Martin Köger
^ permalink raw reply
* Re: [PATCH] git-cvsimport: add support for cvs pserver password scrambling.
From: Dirk Hörner @ 2009-08-14 6:17 UTC (permalink / raw)
To: Sverre Rabbelier
Cc: Johannes Schindelin, Junio C Hamano, Nanako Shiraishi, git
In-Reply-To: <fabb9a1e0908131304m53ab2a68p9faade35969add5@mail.gmail.com>
Hi Sverre,
thanks for the heads up, I will resend it in a minute.
Ciao,
Dirk
2009/8/13 Sverre Rabbelier <srabbelier@gmail.com>:
> Heya,
>
> 2009/8/13 Dirk Hörner <dirker@gmail.com>:
>> sorry for the long delay, but I finally sat down, hacked two testcases
>> and amended the patch after rebasing to the most recent HEAD. Find it
>> attached to this mail.
>
> I think we'd rather find it inlined, as per SubmittingPatches ;).
>
> --
> Cheers,
>
> Sverre Rabbelier
>
^ permalink raw reply
* Re: [PATCH] Work around performance bug in subprocess.Popen.communicate()
From: Karl Wiberg @ 2009-08-14 6:21 UTC (permalink / raw)
To: Catalin Marinas; +Cc: mandolaerik, git
In-Reply-To: <b0943d9e0908131518i3ac18331leb4c0c76313b0780@mail.gmail.com>
On 2009-08-13 23:18:59 +0100, Catalin Marinas wrote:
> 2009/8/4 Karl Wiberg <kha@virtutech.com>:
>
> > On 2009-07-31 12:27:53 +0100, Catalin Marinas wrote:
> >
> > > But can this not lead to a deadlock if the __indata is big? The
> > > stdout of the created process is only processed once the whole
> > > __indata is written. I thought communicate() was created to
> > > avoid this issue.
> >
> > I don't think there's a problem. write() isn't supposed to have a
> > limit on the amount of data it will accept in one call, as far as
> > I'm aware. Plus, it works just fine with Erik's test case---which
> > in my case was about 7 MB. If it can handle 7 MB, I doubt there's
> > a limit we'll hit anytime soon.
>
> write() itself doesn't have a limit, it's mainly what the
> application receiving the data can handle. In the Git case, I think
> it takes all the input as it isn't a filtering tool (things may be
> different with tools like sed etc.).
>
> > Oh, and we still call communicate()---we just don't pass it any
> > additional bytes to write to stdin.
>
> Yes, but if write() is blocked, communicate() won't be called.
Ah, so that's what you were worrying about. Yeah, if the process we're
writing to was going to block us (== not read input) for long periods
of time, we'd have to handle that case. And if it was going to decide
to stop reading input half-way through and give us a sigpipe, we'd
have to handle that. But ...
> Since we are only using Git, I'll merge this patch
Exactly. We really don't expect git to do any of those things to us.
We know it reads all available input before producing the output.
(Well, almost always. I have patches in my experimental branch that
use git cat-file --batch and git diff-tree --stdin, where there can be
many rounds of talking back and forth, but when driving those I'm
being very careful to not deadlock.)
> (and maybe add a comment).
Sure.
--
Karl Wiberg, Virtutech
^ permalink raw reply
* Re: [RFC PATCH 1/2] add a --delete option to git push
From: Sverre Rabbelier @ 2009-08-14 6:24 UTC (permalink / raw)
To: Jeff King; +Cc: Jakub Narebski, Junio C Hamano, Git List
In-Reply-To: <20090814052153.GA2881@coredump.intra.peff.net>
Heya,
On Thu, Aug 13, 2009 at 22:21, Jeff King<peff@peff.net> wrote:
> On Thu, Aug 13, 2009 at 10:05:48PM -0700, Sverre Rabbelier wrote:
> From reading your patch, it looks like it just touches the command-line.
> I think that's the right thing to do, but I think it makes sense to
> think half a second to make sure.
Indeed, the reason I sent out this RFC was to gather more opinions :).
> And with the way you have it, "git push --delete" will silently ignore
> the --delete and push configured refspecs. Probably it should say
> "--delete is useless without refspecs on the command line".
This makes sense, I will fix that.
>> Currently `git push --delete master:master` results in a somewhat
>> cryptic error message. It seems unlikely however, that those new
>> to git would use the 'old:new' notation, so I haven't bothered
>> guarding against it and settled for documenting it.
>
> It seems like it would be simple enough to just check whether the
> refspec contains a colon; if so, silently leave it alone. That could
> also protect configured refspecs, as mentioned above, but I wouldn't
> rule out somebody have a single-name refspec in their config (in fact, I
> think "remote.$X.push = HEAD" is reasonable -- should that delete the
> HEAD on "git push --delete"?).
I don't think we should touch any configured refspecs, think about how
often one would use that vs. the inconvenience of doing so
unintentionally.
>> +--delete::
>> + Delete the specified refs. Prefixes all refs with ':' to tell the
>> + push machinery to delete the specified ref. As such, the refs
>> + that are to be deleted should not contain a ':' specifier.
>> +
>
> This impacts _all_ refspecs. Remember that we can have multiple refspecs
> on the command-line. So I can "move" a ref remotely with:
Correct, hence the plural 'refs'.
> git push :old-name old-name:new-name
>
> but I can't do:
>
> git push --delete old-name old-name:new-name
I don't think that's the use case for this option, it is mostly for
new users who do not know about the colon notation; now you do raise a
valid point that we might want to add a 'git push --rename old new' at
some point, but I think that's beyond the scope of this patch.
> So maybe it would make more sense for it to be "--delete <ref>" and
> impact only a single ref. The simple case of "git push --delete foo"
> would remain unchanged.
I thought about that, but I decided that it was both intuitive and
convenient to be able to delete multiple refs this way.
> The counter-argument is that "--delete" does not necessarily need to be
> as powerful as the ":ref" syntax, but I don't see the downside in making
> it so.
I do, it's easy to make mistakes when it's more powerful, and I think
less intuitive. I think we want this to be as intuitive as possible.
I'm not very opinionated over any of this, if you have strong feelings
yourself please let me know and I'll change the patch.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH] Fix "unpack-objects --strict"
From: Junio C Hamano @ 2009-08-14 6:32 UTC (permalink / raw)
To: Martin Koegler; +Cc: Frank Lichtenheld, git
In-Reply-To: <20090814060307.GA31721@auto.tuwien.ac.at>
Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:
> To introduce a object with one of its linked objects missing, left it
> out of the pack and push it into the repository. unpack-objects will
> unpack all objects and fail updating the ref (but leave all objects in
> the repository). As second step, simply send a ref update request,
> which should succed, as the object is present in the repository.
Your "ref update request" exploit does not work because your understanding
of how we decide to allow updating a ref is flawed.
We do not blindly update a ref to a commit only because we happen to have
that commit. We require that commit to reach existing tips of refs
without break. The logic is in quickfetch() in builtin-fetch.c.
This stronger validation is necessary to deal with any failed transfer by
http walkers, so it is nothing unusual nor new. They walk from the latest
commits that exist on the other side, and their object transfer can be
interrupted before they transfer enough older commits to make the history
connected with what we already had. In such a case we obviously do not
update any ref. And when we re-run the same request, we do not say "Ah,
we have the tip in the object database, so we will update the ref to it".
That is why I said requiring connectivity is a good idea, but keeping them
in core is a misguided waste of memory.
^ permalink raw reply
* Re: [RFC PATCH 1/2] add a --delete option to git push
From: Jeff King @ 2009-08-14 6:33 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Jakub Narebski, Junio C Hamano, Git List
In-Reply-To: <fabb9a1e0908132324td6869aydc752f67b95546f1@mail.gmail.com>
On Thu, Aug 13, 2009 at 11:24:05PM -0700, Sverre Rabbelier wrote:
> > It seems like it would be simple enough to just check whether the
> > refspec contains a colon; if so, silently leave it alone. That could
> > also protect configured refspecs, as mentioned above, but I wouldn't
> > rule out somebody have a single-name refspec in their config (in fact, I
> > think "remote.$X.push = HEAD" is reasonable -- should that delete the
> > HEAD on "git push --delete"?).
>
> I don't think we should touch any configured refspecs, think about how
> often one would use that vs. the inconvenience of doing so
> unintentionally.
I think you are right. My previous message was sort of thinking out
loud, but I think on the whole, the annoyance caused by accidental
deletion is not worth it. :)
> > So maybe it would make more sense for it to be "--delete <ref>" and
> > impact only a single ref. The simple case of "git push --delete foo"
> > would remain unchanged.
>
> I thought about that, but I decided that it was both intuitive and
> convenient to be able to delete multiple refs this way.
> [...]
> I do, it's easy to make mistakes when it's more powerful, and I think
> less intuitive. I think we want this to be as intuitive as possible.
I guess I find what you are doing _more_ complex, because you are really
introducing a whole new mode to push, which is "I am deleting some
stuff". As opposed to some syntactic sugar to replace the confusing
":ref" syntax, which is what I thought the goal was.
On the other hand, "--delete <ref>" introduces its own syntactic
problems. Is it an option, in which case you end up doing:
git push --delete master origin
which is a bit backwards from the usual syntax. Or is it part of the
refspec list, in which case:
1. We have just disallowed a refspec called "--delete" (though to be
fair, you have to be a little insane to use that anyway, and you
can always call it refs/heads/--delete)).
2. Now we don't simply have a list, one refspec per element, which
makes things syntactically a little more complex.
Perhaps saying that "--delete=<ref>" is equivalent to ":<ref>" would be
a reasonable way of adding just the syntactic sugar. I.e.:
git push origin --delete=master
Of course, maybe the goal of a "delete mode" is useful to people. I
can't think of a time when I would have used it, but then I also tend to
think ":<ref>" is elegant and obvious. ;)
I dunno. I don't feel too strongly about it; mainly I was just surprised
because I would have done it the other way. :)
-Peff
^ 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