Unable to checkout table if branch name and table name are identical

[coffeegoddd]

Skipped bats test for this included here: https://github.com/liquidata-inc/dolt/pull/482

After modifying a table, dolt status outputs: (use "dolt checkout <table>" to discard changes in working directory)

If a branch and table share the same name, if you modify the table, then want to use dolt checkout to "discard changes", Dolt thinks the branch is being referenced, outputting: Already on branch <branch>

[datatalking]

While this sounds good on the face, in my experience you usually don't want to combine or mix table names and branch name schema... but I'm curious as to your case.

Can you give me some cases on why this would be a good idea, seems like you wouln'dnt want to have dual names so seems like we would be causing a bigger problem than solving one.

[timsehn]

I think we want to do the exact same thing as Git in this circumstance.

Yes. Bad idea. But we need to match what the reference implementation does :-)

[bpf120]

@datatalking Thanks for commenting. Are you looking at Dolt for any projects? We like learning what potential users are up to. :) Happy to chat over email or on our Discord.

[codeaucafe]

Any case this still needs a fix? Happy to help out if so.

[nicktobey]

I think this has already been fixed.

If you want to drop changes from the working set but not staging, you can do dolt checkout -- tablename, which should work even if the table name matches a branch name.

If you want to drop changes from both staging and the working set, you can do dolt checkout HEAD -- tablename.

This distinction is confusing, but it matches Git's behavior.

[nicktobey]

Actually, looking at this more closely I don't think this is fixed.

dolt checkout seems to not be implemented correctly. It ignores -- completely, and always interprets the first other parameter as a branch name (or other refspec).

dolt reset has similar issues, but worse. It didn't seem to handle the case of providing both a branch/refspec and a list of tables at all: If multiple parameters are passed, it simply resets every table to match HEAD, which is clearly not the intended behavior.

@codeaucafe if you still want to tackle this, by all means.

[nicktobey]

This might be tricky to get right.

Some context you might need:

The command is implemented in a file named dolt_checkout.go

Assuming we want to match Git's behavior (@coffeegoddd feel free to push back if you don't think we should), then the intended behavior is as follows:

1. dolt checkout branchname -- tables... - updates the named tables such that both the staged changes and the working set match the value that tables had at the named branch.
2. dolt checkout -- tables... - drops unstaged changes on the named tables (basically, update the working set in the named tables to match the currently staged changes)
3. dolt checkout branchname tables... - If branchname matches a valid branch, same as (1). Otherwise, treat it as a table name and do the same as (2)
4. dolt checkout branchname -- - updates every table such that both the staged changes and the working set match the value that tables had at the named branch.
5. dolt checkout branchname - If branchname matches a valid branch, same as (4). Otherwise, treat it as a table name and do the same as (2)

I wouldn't worry too much about what "updating tables" looks like: the current implementation in dolt_checkout should call a method that handles that, we just need to make sure the right parameters are passed to that method.

There's also another entrypoint in a file named checkout.go that ends up calling the version in dolt_checkout.go. It's supposed to forward all the command line arguments, but I have a suspicion it's stripping out the -- flag: our argument parser handles it specially. It doesn't appear in the list of positional arguments. Instead, the ArgParseResults struct has an int field containing the index it encountered --. So we need to make sure that when checkout.go generates the args that it passes to dolt_checkout.go, it doesn't get dropped.

[codeaucafe]

Thanks for the info @nicktobey. FYI I was looking for other issues to work on and asked about this one and 1083 (also an import issue like I did last week). I was going to work on that first, SO if someone wants to work on in meantime they can. If not then I'll work on this after 1083, if that is fine with you.

Let me know if that doesn't work.

Thank you

[nicktobey]

Of course. This isn't high priority: the fact that the command isn't properly handling this case (using both a branch name + table name) is concerning, but it's a very uncommon use case.

The fix for this also touches multiple parts of the code in ways that aren't the most straightforward. Doing 1083 first will probably provide context that will help with this issue.

[elianddb]

I'll provide the recreation instructions for this error.

1. Create a remote repository and with a new branch named test.
2. Clone the repository and stage/commit a file with the same name under the main branch.
3. Attempt to checkout to test.

git clone ... && cd ...
echo test > test
git add . && git commit -m "add test"

git checkout test
fatal: 'test' could be both a local file and a tracking branch.
Please use -- (and optionally --no-guess) to disambiguate

[codeaucafe]

Thank you! I'll start working on this in the next week or so.

[elianddb]

Thank you! I'll start working on this in the next week or so.

I'm in the process of a fix right now so no worries.