Re: [PATCH] Teach git-checkout-index to use file suffixes.

Shawn Pearce [off-list ref] writes:

Why let the user choose?  '/' shouldn't be permitted as that
(possibly) would try to create a directory also named the same as
the file (a bad thing).  Then its just two formats, depending on
if you want all stages or just 1 stage:

	--stage=all --suffix=#1/#2/#3

or

	--stage=1 --suffix=#1

I'll later agree with "why let the user choose" in this message,
but for an entirely different reason.

I am not a great fan of extracting files from stages in the
working tree to begin with.  The git-unpack-file command tries
carefully to avoid conflicts, and its use of temporary files is
inherently, eh, temporary, and Porcelains know what names they
get and know how to clean things up afterwards.

I am sympathetic to what you are trying to achieve here.  Maybe
your Porcelain would invoke a graphical 3-way merge program in a
windowing environment, and the merge program displays the
filename(s) your Porcelain fed it to the end user, without
giving you a way to override it.  In that context, it is not
useful to the end user to show meaningless temporary filename,
and having checkout-index to derive the temporary filename from
real filename may look sensible.

However, --stage=all with --suffix would introduce name clashes
between repeated conflicted merge runs, which requires Porcelain
to be extra careful.  Your last merge run might have involved
three conflicting stages (leaving a.c~1, a.c~2, and a.c~3 in
your working tree) and this time it may be "we removed it while
they modified it" situation (needing to extract a.c~1, a.c~3 but
not a.c~2).  The Porcelain needs to make sure not to get
confused by leftover a.c~2 file in the working tree from the
previous run.

If what you are trying is to reduce the number of checkout-index
calls by your Porcelain to extract conflicted stages, it _might_
make more sense to do something like this instead (I am thinking
aloud, so take this with a big grain of salt -- it may not make
sense at all):

    checkout-index --stage=all checks out higher-order stages in
    made-up temporary filenames, just like git-merge-one-file
    does using git-unpack-file one-by-one, with a single
    invocation.

    It reports the following to its standard output, one record
    per pathname in the index:

	tmp1 <SP> tmp2 <SP> tmp3 <TAB> pathname <RS>

    Here, tmp? is the temporary file names for each stage, or a
    dot for for a nonexistent stage.  pathname is c-quoted as
    usual for non -z output and RS is '\n' (under -z flag,
    pathname is a literal string and RS is NUL).  SP = space
    (ASCII 0x20), TAB = tab (ASCII 0x09).

    Example:

        ".tmp1323 . .tmp1232\tfoo/a.c\n"

    Notice the dot between the two temporary files -- stage2 is
    empty in this example.

Then your Porcelain could run --stage=all and read the output,
and then moving the temporary files around whatever way pleases
it.  Since it needs to be careful about not overwriting the
existing working tree files *and* not getting confused by
existing garbage in the working tree anyway, I do not think it
would make your life much easier to have checkout-index derive
the temporary file names after the real working tree files.

BTW, using TAB to split metainformation and pathname and using
SP to separate metainformation pieces are in line with the
design other git tools use.  With this, you can separate the LHS
metainformation part and pathname using "cut", and you can have
shell IFS to split the metainformation part apart after that.
But that is a minor detail.

Junio C Hamano [off-list ref] wrote:
[...]

However, --stage=all with --suffix would introduce name clashes
between repeated conflicted merge runs, which requires Porcelain
to be extra careful.  Your last merge run might have involved
three conflicting stages (leaving a.c~1, a.c~2, and a.c~3 in
your working tree) and this time it may be "we removed it while
they modified it" situation (needing to extract a.c~1, a.c~3 but
not a.c~2).  The Porcelain needs to make sure not to get
confused by leftover a.c~2 file in the working tree from the
previous run.

Clearly.  pg was trying to delete all of those files before doing its
merge work but failed because of the --ignored bug in git-ls-files;
but this is now fixed.  Still a possibility for confusion does exist.

But in at least one case my Porcelain is Eclipse and a Cygwin
prompt.  In this case I want to view and edit everything in Eclipse.
Having all of the files in the same directory just makes it easier
to view.  Random temporary names in the same directory as the tracked
file would be OK except if I had multiple conflicts in the same
directory, in which case I need some easy way to tell them apart.
At which point we're starting to derive off the tracked file name
and might as well always use well-known names.
 

If what you are trying is to reduce the number of checkout-index
calls by your Porcelain to extract conflicted stages, it _might_
make more sense to do something like this instead (I am thinking
aloud, so take this with a big grain of salt -- it may not make
sense at all):

    checkout-index --stage=all checks out higher-order stages in
    made-up temporary filenames, just like git-merge-one-file
    does using git-unpack-file one-by-one, with a single
    invocation.

    It reports the following to its standard output, one record
    per pathname in the index:

	tmp1 <SP> tmp2 <SP> tmp3 <TAB> pathname <RS>

[...]

That's not a bad idea.  The only thing I don't like about that
is that git-checkout-index won't build the directory tree for me;
the Porcelain must still be responsible for doing that before it
can rename the temporary files (if available) into the correct
subdirectory.

My plan with git-checkout-index though was originally to just have
it fail if the file already exists, unless -f is given.  So if
a left-over foo.c#2 was still on disk and git-checkout-index was
going to write to that name it would fail.


I see a lot of benefit from the checkout to temporary file names
and let the Porcelian rename (if it desires).

So I'm going to ask you to withdraw the --suffix patch from pu.
I'll write up a new patch using the ideas you suggest above and
submit that instead.

-- 
Shawn.